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.
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(dynamic_config): Add config for dynamic metrics extraction #2252
feat(dynamic_config): Add config for dynamic metrics extraction #2252
Changes from all commits
abbfa60
f60b6c9
6ce7ce5
9b1b2bb
599768b
7110eaf
37c46bf
1b897cf
2267a39
9c4f505
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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 don't really like
TagMapping
, but don't really have better alternatives. What do you think aboutSharedTags
?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.
We could do
SharedTagMapping
, would that be better?Note: I might merge this PR sooner, but we can follow up with some cleanup.
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.
So this is the false statement here, if both specified we always return
field
as asource
on line 255.Or will be there more code which will make this behavior undefined ?
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.
My intent was to leave behavior undefined on the protocol level (hence the doc comment), but of course we need to do something. At this point, I'd like to leave our options open to throw an error, use both as a fallback, or just have one take precedence over the other. Either way, at this point we do not expect Sentry to emit entries with both set.
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.
What do you think about logging an error in this case, at least for the first few iterations?
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.
If we make changes during our first iterations, this error would also show up for customers if they run a Relay with this intermediate version. Since this is future-proofing, it's better if we keep this silent. There can be other tests that ensure our config roundtrips in Sentry.