Add ActiveSupport tagged logging support #1344
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
To temporarily set the attributes in the logger instance, use instance variables instead of Thread variables. I don't know why Thread variables are used here. I don't think it's needed.
tombruijn
force-pushed
the
tagged-logging
branch
from
f5c7638 to
72955eb
Compare
Our logger did not support Rails ActiveSupport tagged logging yet. This was because our logger didn't listen to the `tagged`, `push_tags` and `pop_tags` methods. Implement these methods to have the logger that wraps our logger call these methods to set tags. The format is the same as Rails formats it: `[My tag value]` I decide not to set these as attributes, because they're only values. They have no key and the value can be anything and change frequently.
tombruijn
force-pushed
the
tagged-logging
branch
from
72955eb to
41e1e8f
Compare
luismiramirez
approved these changes
Dec 5, 2024
This comment has been minimized.
This comment has been minimized.
2 similar comments
This comment has been minimized.
This comment has been minimized.
This is a message from the daily scheduled checks. |
unflxw
added a commit
that referenced
this pull request
Dec 19, 2024
This commit removes tagged logging support from `Appsignal::Logger`, essentially reverting most of the changes in #1344, #1350, #1351 and The change to use instance variables instead of thread variables is maintained, as well as the tests that assert the expected behaviour when used alongside `ActiveSupport::TaggedLogging`, which pass notwithstanding the reverted functionality. This makes explicit in our test suite the way in which tagged logging is supported.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Don't use thread vars in logger
To temporarily set the attributes in the logger instance, use instance variables instead of Thread variables.
I don't know why Thread variables are used here. I don't think it's needed.
Add ActiveSupport tagged logging support
Our logger did not support Rails ActiveSupport tagged logging yet. This was because our logger didn't listen to the
tagged,push_tagsandpop_tagsmethods.Implement these methods to have the logger that wraps our logger call these methods to set tags.
The format is the same as Rails formats it:
[My tag value]I decide not to set these as attributes, because they're only values. They have no key and the value can be anything and change frequently.