De-dot Micrometer metric names #1700
Conversation
This is a breaking change. However, it seems to be a necessary one: The consequences could be quite severe (mapping conflicts may only be fixable by deleting the index). Also, the micrometer integration is still in beta so breaking changes are probably ok. The alternatives, such as flattened fields have downsides, such as lack of Kibana support and worse performance. The Elasticsearch team is not considering flattened fields the endgame and are investigating other alternatives. With that, I don't see the need for a config option to opt-out of the de-doting yet.
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
Trends 🧪💚 Flaky test reportTests succeeded. Expand to view the summary
Test stats 🧪
|
Codecov Report
@@ Coverage Diff @@
## master #1700 +/- ##
============================================
- Coverage 58.86% 58.84% -0.03%
Complexity 92 92
============================================
Files 403 403
Lines 18475 18478 +3
Branches 2570 2572 +2
============================================
- Hits 10875 10873 -2
- Misses 6832 6835 +3
- Partials 768 770 +2
Continue to review full report at Codecov.
|
|
Thanks @felixbarny, I'm excited to see this alignment across our products. Did you implement a configuration flag for backward compatibility? |
|
I've added a |
|
Thanks @felixbarny . I'm not familiar enough with asciidoc and I didn't see |
| private static void serializeValueStart(String key, String suffix, JsonWriter jw, StringBuilder replaceBuilder, boolean dedotMetricName) { | ||
| replaceBuilder.setLength(0); | ||
| if (dedotMetricName) { | ||
| DslJsonSerializer.sanitizeLabelKey(key, replaceBuilder); |
There was a problem hiding this comment.
[minor] maybe rename method as it's now not only used for label keys, but more keys in general.
|
|
||
| @Test | ||
| void testDedotMetricName() { | ||
| meterRegistry.counter("foo.bar").increment(42); |
There was a problem hiding this comment.
[minor] maybe add extra assertion to ensure MetricsConfiguration#isDedotCustomMetrics returns true by default.
|
@cyrille-leclerc The comment from CI contains a handy link to the docs preview: https://apm-agent-java_1700.docs-preview.app.elstc.co/guide/en/apm/agent/java/master/config-metrics.html
|
|
@cyrille-leclerc: I think I misinterpreted your last statement. I just implemented that flag after you asked for it. |
|
Hello @felixbarny - I wanted to propose the feature flag but you did in the meanwhile 👏 I would be tempted to switch its default to |
|
Hey Luca, I was thinking of that but the current behavior ( I know that this is a breaking change and thus can be painful for some users. Those users can use the flag to opt-out (and live in constant danger of mapping conflicts). Note that the Micrometer integration is still in beta which is why I think introducing a breaking change is reasonable. |
|
Thanks @lucabelluccini . As @felixbarny explained, we looked at the pros and cons and we prefer to introduce this backward incompatible change on this beta version with a configuration param to go back to the previous mode:
|
|
Totally agree and I didn't recall it was in |
What does this PR do?
This is a breaking change. However, it seems to be a necessary one:
The consequences could be quite severe
(mapping conflicts may only be fixable by deleting the index).
Also, the micrometer integration is still in beta so breaking changes
are probably ok.
The alternatives, such as flattened fields have downsides, such as
lack of Kibana support and worse performance.
The Elasticsearch team is not considering flattened fields the
endgame and are investigating other alternatives.
With that, I don't see the need for a config option to opt-out of the
de-doting yet.
relates to elastic/apm#347
Checklist