-
Notifications
You must be signed in to change notification settings - Fork 73
DSMON-1102: Add configuration-level dynamic tags for JMX attribute values #581
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
Conversation
6a260b9 to
882f7c8
Compare
882f7c8 to
9961226
Compare
| String tagName = entry.getKey(); | ||
| String tagValue = entry.getValue(); | ||
|
|
||
| if (tagValue != null && tagValue.contains("#") && tagValue.startsWith("$")) { |
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 want to make sure normal tags are not being mistaken for JMX tags, but I think this condition is strict enough that it won't happen. A tag starting with $ is not valid anyway.
Also, the parsing is in a try / catch, the catch adds it to normal 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.
I want to make sure normal tags are not being mistaken for JMX tags,
More robust way would be to have a distinct configuration section for computed tags, and would avoid the need for bespoke syntax.
Also, jmxfetch integrations are configured in yaml, where "#" denotes a comment. Would it be possible to use a different separator? Or perhaps even be explicit and use two keys: bean_name and attribute.
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.
Makes sense! You actually made me think about backward compatibility (I want to make sure users can update their integration configurations without worrying about breaking their integration).
See PR: DataDog/integrations-core#21687
Adding a dynamic_tags section at the same level as the include section is backwards compatible (I can't add the dynamic_tags section inside the include section). I find the current format pretty clean, and backwards compatible.
| String tagName = entry.getKey(); | ||
| String tagValue = entry.getValue(); | ||
|
|
||
| if (tagValue != null && tagValue.contains("#") && tagValue.startsWith("$")) { |
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 want to make sure normal tags are not being mistaken for JMX tags,
More robust way would be to have a distinct configuration section for computed tags, and would avoid the need for bespoke syntax.
Also, jmxfetch integrations are configured in yaml, where "#" denotes a comment. Would it be possible to use a different separator? Or perhaps even be explicit and use two keys: bean_name and attribute.
| return getInclude() != null && !getInclude().isEmptyFilter(); | ||
| } | ||
|
|
||
| /** Resolve dynamic tags for this configuration using cached values. */ |
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.
Could you please make the comment more explicit? What does "Resolve" mean in this case, what are the inputs and outputs?
| Object attrObj = config.get("attribute"); | ||
|
|
||
| if (tagNameObj == null || beanObj == null || attrObj == null) { | ||
| log.warn("Invalid dynamic tag config: missing 'tag_name', 'bean' or 'attribute' key"); |
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.
Let's be explicit what is missing.
|
|
||
| Map<String, Object> config = (Map<String, Object>) tagConfig; | ||
| Object tagNameObj = config.get("tag_name"); | ||
| Object beanObj = config.get("bean"); |
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 may be wrong, but I think this refers to the same string as bean_name in the filter section. Would it make sense to use the same key name here?
| Object beanObj = config.get("bean"); | |
| Object beanObj = config.get("bean_name"); |
| private Filter include; | ||
| private Filter exclude; | ||
| private List<DynamicTag> dynamicTags = null; | ||
| private Map<String, String> resolvedDynamicTags = null; |
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.
Up until now, this object represented only user configuration with no runtime data. Would it be possible to keep it this way?
We can keep any runtime data inside Instance and JmxAttribute. For example JmxAttribute can retrieve tag values from the instance cache once we found a matching configuration.
| /** Sets a matching configuration for the attribute. */ | ||
| public void setMatchingConf(Configuration matchingConf) { | ||
| public void setMatchingConf(Configuration matchingConf, | ||
| Map<String, String> resolvedDynamicTags) { |
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.
Let's use a separate setter for dynamic tags.
| } | ||
|
|
||
| for (DynamicTag dynamicTag : allDynamicTags) { | ||
| String cacheKey = dynamicTag.getBeanName() + "#" + dynamicTag.getAttributeName(); |
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.
This is duplicated here and in the next method. Would it be possible to make it a method of DynamicTag?
| if (!this.dynamicTagsCache.containsKey(cacheKey)) { | ||
| Map.Entry<String, String> resolved = dynamicTag.resolve(connection); | ||
| if (resolved != null) { | ||
| this.dynamicTagsCache.put(cacheKey, resolved); |
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 dynamic tag references a non-existing attribute, we would try to retrieve it multiple times (once for each config it is part of). Would it be possible to cache negative lookups as well, so we don't spam the logs with identical errors?
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
Problem
Kafka exposes its cluster ID via JMX, but as an attribute value rather than in the bean path:
kafka.server:type=KafkaServer,name=ClusterIdValue→ returns the actual cluster ID (e.g.,"kafka-prod-cluster-01")Currently, there's no way to extract this value and tag all Kafka metrics with it. Users must either:
This becomes particularly problematic when monitoring multiple Kafka clusters or across different environments (dev/staging/prod).
Solution
This PR adds support for configuration-level dynamic tags that can extract values from JMX bean attributes at runtime and apply them to all matching metrics.
Syntax:
Example: Kafka Cluster ID
All Kafka metrics automatically tagged with the cluster ID: