-
Notifications
You must be signed in to change notification settings - Fork 317
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
add remote config support for custom tags #3875
Conversation
Overall package sizeSelf size: 5.78 MB Dependency sizes
🤖 This report was automatically generated by heaviest-objects-in-the-universe |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #3875 +/- ##
==========================================
- Coverage 84.73% 84.66% -0.07%
==========================================
Files 237 237
Lines 9937 9967 +30
Branches 33 33
==========================================
+ Hits 8420 8439 +19
- Misses 1517 1528 +11 ☔ View full report in Codecov by Sentry. |
d2456b3
to
a12d59c
Compare
logInjection: 'DD_LOG_INJECTION', | ||
headerTags: 'DD_TRACE_HEADER_TAGS', | ||
tags: 'DD_TAGS' | ||
} |
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.
The config value names will be normalized when sent to the backend.
sampleRate
and DD_TRACE_SAMPLE_RATE
map to the same thing, logInjection
is accepted but DD_LOG_INJECTION
won't be recognized. We need to create a PR to config_norm_rules so that headerTags
will be accepted. DD_TAGS
will be accepted but tags
won't be recognized. Since we will be sending the other config values to the backend with camel case names, I'm suggesting we do that here too, and create a PR to the json file to add the missing mappings.
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.
Not sure I understand. Are you saying that the value that is literally used by the backend to send the information to us will not be recognized by the same backend when reported as telemetry? If that's the case it sounds like a bug in the backend that should be fixed, and thus I don't think it's on us to change anything in another PR. The safe approach is to send the same names we received.
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 asked about this and was told this is all still in flight and not required to be correct for now. System tests will be added when we know exactly the values that need to be sent.
5c68135
to
1bf591a
Compare
1bf591a
to
d6f8681
Compare
What does this PR do?
Add remote config support for custom tags.
Motivation
So that users can set custom tags through the UI.
Additional Notes
The previous behaviour of merging tags was incorrect and has now been fixed with the tests updated accordingly.