Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement logging in Dash-Seedling 151 #10

Merged
merged 16 commits into from
Apr 21, 2023

Conversation

jedheaj314
Copy link
Member

@jedheaj314 jedheaj314 commented Apr 20, 2023

Resolves #151

  • Implements logging capability in Dash-Seedling app.
  • Logs basic operation info messages in Dash-Seedling app.
  • Consumes existing app insights environment variables (from app service config)
  • Local monitoring support enabled through providing APPLICATIONINSIGHTS_CONNECTION_STRING env var
  • Ensures logs generated by Dash-Seedling can be viewed in Application Insights on Azure.

Test Passed :

  • Basic - Opens without error in dev container VS code.
  • App container runs successfully.
  • app.py runs successfully.
  • With environment variable set with value in .env - logs can be seen in application insights.

Copy link
Member

@martinpeck martinpeck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small "nit picking" comment, but otherwise LGTM.

Do you need to add the APPLICATIONINSIGHTS_CONNECTION_STRING into the deployment in a separate PR?

Copy link
Member

@t-young31 t-young31 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think APPLICATIONINSIGHTS_CONNECTION_STRING should be set for local, but apart from that – LGTM!

I would have

if os.environ.get("ENVIRONMENT", default="dev") == "local":
    return

in initialise_logging

@jedheaj314
Copy link
Member Author

jedheaj314 commented Apr 20, 2023

A

Small "nit picking" comment, but otherwise LGTM.

Do you need to add the APPLICATIONINSIGHTS_CONNECTION_STRING into the deployment in a separate PR?

APPLICATIONINSIGHTS_CONNECTION_STRING
this is already set in the previous changes outside this PR, just discussed this with James and no change is needed related to this in this PR.

@jedheaj314 jedheaj314 marked this pull request as draft April 20, 2023 15:48
@jedheaj314 jedheaj314 requested a review from t-young31 April 21, 2023 10:12
@jedheaj314 jedheaj314 marked this pull request as ready for review April 21, 2023 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants