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

feat: global context for logging #25920

Closed
wants to merge 2 commits into from

Conversation

eschutho
Copy link
Member

@eschutho eschutho commented Nov 9, 2023

SUMMARY

We have a few examples in the codebase where for logging purposes we need to pass data down to parts of the codebase that have no need to know about this data. It creates very coupled and bloated modules that often are just passing data down without even acting on it. This change proposes the use of a global context dictionary that is used for read-only purposes for logging.

There are three places where we can use this functionality. One is an open PR that I wrote that helps us to log a report execution id to tie together multiple logs to the same report execution.
Another example is passing report header data to an smtp event for logging in a mail server.
And the third use case is to pass dashboard_id and chart_id into the sql mutator for logging.

I have rewritten the first two examples each in one commit in this PR for diffing the implementations for each with a global context. I'll add how the third example (sql mutator) either in this PR or in a subsequent one.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@eschutho eschutho force-pushed the elizabeth/global-context branch from f98009c to d97a7fa Compare November 9, 2023 00:32
@john-bodley
Copy link
Member

@eschutho out of interest what's the criteria for logging additional context and is there any concern about how much additional data we're passing over the wire? How do we resolve situations where different organizations have different requirements?

@eschutho
Copy link
Member Author

Thanks @john-bodley I'd love to get some early feedback on this idea. As I'm refactoring existing code, I've been targeting that one constraint would be to only store ids in the global context. When logging, the module responsible for logging can fetch the global context id and then any relevant information from the metadata that it needs. The context doesn't need to store or know about all the data that the logger needs, just the minimum requirements.

My main concern is more around the global context getting messy and overused. I don't think there are issues about sending information over a network request, since the context should not be returned in any api responses, but I don't want it to become a dumping ground for passing around any kind of state because that would be an antipattern. Instead, if we limit it to single resource ids, or perhaps a list of ids, I think that could get us where we need for now, and we can reevaluate if other situations arise where we feel we would need this.

@eschutho eschutho force-pushed the elizabeth/global-context branch from d97a7fa to 3b7ee49 Compare November 17, 2023 01:36
@eschutho eschutho force-pushed the elizabeth/global-context branch from 3b7ee49 to 6ae7967 Compare November 17, 2023 23:56
@eschutho eschutho mentioned this pull request Jan 6, 2024
9 tasks
@eschutho
Copy link
Member Author

eschutho commented Jan 8, 2024

I'm closing this and splitting it into smaller PRs.

@eschutho eschutho closed this Jan 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants