-
Notifications
You must be signed in to change notification settings - Fork 567
fix: apply filtering EmitLogRecord #4079
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
Open
proost
wants to merge
20
commits into
open-telemetry:main
Choose a base branch
from
proost:fix-apply-filtering-emitlogrecord
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 12 commits
Commits
Show all changes
20 commits
Select commit
Hold shift + click to select a range
c7360f5
fix: apply filtering in emit log record
proost 46c13c8
refactor: filter once
proost 849c52f
Merge branch 'fix-apply-filtering-emitlogrecord' of https://github.co…
proost 47d1e37
refactor: enabled filtering on the API level
proost a0958a4
feat: use context for EmitLogRecord
proost 355ac95
Merge branch 'main' of github.com:open-telemetry/opentelemetry-cpp in…
proost 98b3e9c
doc: update changelog
proost 05b865b
style: follow doc lint
proost 8063cb6
doc: follow doc lint
proost efa0202
Merge branch 'fix-apply-filtering-emitlogrecord' of https://github.co…
proost 17de264
Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…
proost b9871a4
style: follow doc lint
proost 5e67680
perf: cached enabled required
proost 80051a4
Merge branch 'fix-apply-filtering-emitlogrecord' of github.com:proost…
proost ff7e302
Merge branch 'main' into fix-apply-filtering-emitlogrecord
ThomsonTan dc306d5
feat: has enabled filter
proost 1a8111f
Merge branch 'fix-apply-filtering-emitlogrecord' of github.com:proost…
proost 89c8209
Merge branch 'main' of github.com:open-telemetry/opentelemetry-cpp in…
proost d4578ad
feat: use temp context
proost d62c8e4
refactor: unify function
proost File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we avoid making every enabled log call pay for the full
Enabled(...)chain? The disabled and below-min-severity paths stay cheap, but the normal enabled path now pays virtual/context/processor cost before record creation. It may be better to cache whether full filtering is actually needed, and only call the full chain when trace-based filtering or processor-level filtering is configured.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thx, 5e67680
how about go through trace based filtering using current option?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this helps with the trace-based case, but I think the original concern is only partially addressed. We still need the cache to account for processor-level filtering too, otherwise the normal non-trace-based path can skip processor
Enabled(...)checks.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, now i understand your intention.
dc306d5
Later, i will add logger configuration.. this option might be needed in the configuration. But this is not in the spec. is ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not add a new logger configuration option just for this. The spec defines the observable Enabled behavior, but the C++ SDK can still implement it efficiently.
For performance, I think this should stay as an internal cache/capability decision: full filtering is needed when trace_based is enabled or when a processor actually implements Enabled filtering. Otherwise the normal enabled path should avoid paying the full virtual/context/processor chain.
One thing to watch is that the cache must stay correct if processors are added after a logger is created.