Skip to content
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

[break] Fix MetricName comparator for tags, MetricName#safeTags() returns SortedMap #272

Merged
merged 5 commits into from
May 26, 2019

Conversation

schlosna
Copy link
Contributor

@schlosna schlosna commented May 24, 2019

Ensure MetricName#safeTags are compared in a total ordered fashion.

Ensure MetricName#safeTags are compared in a total ordered fashion.
@schlosna schlosna requested a review from carterkozak May 24, 2019 20:37
@@ -65,7 +44,7 @@

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually should make this:

    @Value.NaturalOrder
    NavigableMap<String, String> safeTags();

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would do the trick, if we're okay with exposing NavigatableMap instead of Map

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does MetricName need to be comparable? Are we better off removing that piece?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mainly added it for convenience for the toString form when analyzing getMetrics output

Map.Entry.<String, String>comparingByKey()
.thenComparing(Map.Entry.comparingByValue());

private static final Comparator<Map<String, String>> sizeComparator = Comparator.comparingInt(Map::size);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice touch

@@ -61,11 +40,12 @@
* <p>
* All tags and keys must be {@link Safe} to log.
*/
Map<String, String> safeTags();
@Value.NaturalOrder
NavigableMap<String, String> safeTags();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we loosen this to SortedMap?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to just leave this as a NavigableMap as the underlying impl generated by immutables is a TreeMap either way with jdkOnly = true (though we could remove that since we already rely on Guava implementation dependency, created optional #273 for that).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer the guava approach. We heavily rely on guava immutable collections, and they will avoid collection re-allocations when we attempt to re-wrap them.

I'm not sure any of the functionality provided by NavigableMap will be helpful for safeTags, where SortedMap provides clear advantages. Using the less specific type allows more freedom to change the implementation in the future. That said, I'm not actually aware of a SortedMap implementation which doesn't also implement NavigableMap, so I defer to your judgement.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, you convinced me, went with SortedMap for now

Copy link
Contributor Author

@schlosna schlosna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Introducing a slight break since 0.12.5 by removing Comparable from MetricName, but I don't think that was actually necessary and was just something I had added for my own convenience when implementing the tagged cache metrics.

@@ -61,11 +40,12 @@
* <p>
* All tags and keys must be {@link Safe} to log.
*/
Map<String, String> safeTags();
@Value.NaturalOrder
NavigableMap<String, String> safeTags();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to just leave this as a NavigableMap as the underlying impl generated by immutables is a TreeMap either way with jdkOnly = true (though we could remove that since we already rely on Guava implementation dependency, created optional #273 for that).

@bulldozer-bot bulldozer-bot bot merged commit c2b1222 into develop May 26, 2019
@bulldozer-bot bulldozer-bot bot deleted the ds/metric-name-tags-comparator branch May 26, 2019 21:46
@carterkozak
Copy link
Contributor

Thanks!

@iamdanfox iamdanfox changed the title Fix MetricName comparator for tags [break] Fix MetricName comparator for tags, MetricName#safeTags() returns SortedMap May 28, 2019
@iamdanfox
Copy link
Contributor

As mentioned by @JacekLach, automatically upgrading to this version of tritium has caused NoSuchMethodErrors at runtime! It seems like changing Map -> SortedMap was API compatible but not ABI compatible!

We had some code in sls-logging, which did this:

        Map<String, String> tags = metricName.safeTags();

        if (metric instanceof Counter) {
            return Optional.of(MetricLogs.v1counter(name, (Counter) metric, tags));
        } else if (metric instanceof Meter) {
            ...

And it seems that first line triggers the exception:

java.lang.NoSuchMethodError: com.palantir.tritium.metrics.registry.MetricName.safeTags()Ljava/util/Map;
	at com.palantir.sls.logging.metric.ScheduledTaggedMetricLogReporter.metricLog(ScheduledTaggedMetricLogReporter.java:121)
	at com.palantir.sls.logging.metric.ScheduledTaggedMetricLogReporter.reportMetrics(ScheduledTaggedMetricLogReporter.java:102)
	at com.palantir.sls.logging.metric.ScheduledTaggedMetricLogReporter.lambda$report$2(ScheduledTaggedMetricLogReporter.java:91)
	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:515)
	at com.palantir.tracing.DeferredTracer.withTrace(DeferredTracer.java:115)
	at com.palantir.tracing.Tracers$TracingAwareCallable.call(Tracers.java:343)
	at com.palantir.tracing.WrappingExecutorService.lambda$wrapTask$0(WrappingExecutorService.java:67)
	at com.codahale.metrics.InstrumentedExecutorService$InstrumentedRunnable.run(InstrumentedExecutorService.java:176)
	at com.palantir.tritium.metrics.TaggedMetricsExecutorService$TaggedMetricsRunnable.run(TaggedMetricsExecutorService.java:161)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
	at java.lang.Thread.run(Thread.java:834)

We worked around this by just releasing a new version of sls-logging.

@schlosna
Copy link
Contributor Author

@iamdanfox sorry about that ABI break. I'm open to reverting that change if necessary and tagging a 0.13.1 (I'm half inclined to just remove immutables generation for MetricName as well so that I can fully control the generated implementation).

We should probably consider an actual 1.0.0 so that we can effectively do real API/ABI breaks and mark them as proper semver major version bumps.

@carterkozak
Copy link
Contributor

I would support removing immutables since I've found a few places that subclass MetricName, I would prefer if we provided a concrete final implementation instead. interface -> class would be another ABI break though.

@schlosna
Copy link
Contributor Author

I would probably leave MetricName as an interface, but provide an ABI compatible Immutable builder and simple static factory for just name & name+tags.

@iamdanfox
Copy link
Contributor

Given how easy it was to workaround this, not sure it's worth reverting just yet, just wanted to flag because this is such a low level library that even tiny breaks can end up having quite far reaching consequences!

@JacekLach
Copy link

It was an easy workaround, but we only caught it in prod (when we noticed we were missing a subset of metrics for ~12h)! So there's a bit more to the tradeoff than just how difficult/easy the fix was.

CRogers pushed a commit that referenced this pull request Sep 11, 2019
Ensure `MetricName#safeTags` are compared in a total ordered fashion.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants