-
Notifications
You must be signed in to change notification settings - Fork 399
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: Adds new configuration options to add custom tags (labels) to logs #2743
feat: Adds new configuration options to add custom tags (labels) to logs #2743
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2743 +/- ##
==========================================
- Coverage 97.25% 97.21% -0.05%
==========================================
Files 294 294
Lines 46233 46310 +77
==========================================
+ Hits 44965 45020 +55
- Misses 1268 1290 +22
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
…on config as `parsedLabels` and `loggingLabels`, moved tests around
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'm confused as to why we apply labels to all logs instead of providing a way for users to add labels to specific logs, but the implementation looks sound to me.
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.
approving but since I made updates on @svetlanabrennan's behalf, I'll wait for @jsumners-nr to approve as well
lib/config/index.js
Outdated
* Compares the labels list to the excluded label list and removes any labels that need to be excluded. | ||
* Then prefixing each label with "tags." | ||
*/ | ||
Config.prototype._filterLabels = function filterLabels() { |
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'd add some tests in config unit tests that assert the labels are getting set accordingly. I know they are being tested in the log aggregator but it'll be hard to identified where this is getting covered in the future
lib/aggregators/log-aggregator.js
Outdated
@@ -24,6 +28,7 @@ class LogAggregator extends EventAggregator { | |||
|
|||
super(opts, agent) | |||
this.agent = agent | |||
createFeatureUsageMetrics(agent) |
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 were missing some supportability metrics per spec. This story was supposed to add the Supportability/Logging/Labels/Nodejs/<enabled|disabled>
Description
How to Test
Run unit tests.
Related Issues
Closes #2716
TODO