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

Use Tag#compareTo to detect duplicates #2764

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

datibbaw
Copy link

@datibbaw datibbaw commented Aug 27, 2021

Addresses #1432 by using the compareTo() method from the Tag interface, defined here

default int compareTo(Tag o) {
    return getKey().compareTo(o.getKey());
}

The above is already marked as overridable and implements the existing dedup logic almost exactly:

for (int i = 0; i < n - 1; i++)
    if (!tags[i].getKey().equals(tags[i + 1].getKey()))
        tags[j++] = tags[i];

In this way, developers can extend ImmutableTag, which implements Tag, with their own class to override the compareTo() method like so.

@Override
public int compareTo(Tag o) {
  return Comparator.comparing(Tag::getKey).thenComparing(Tag::getValue).compare(this, o);
}

To be clear, this isn't meant to be made available by default; because of its limited applicability, developers take on the risk of passing the appropriate values to the corresponding backend.

@datibbaw datibbaw marked this pull request as ready for review August 27, 2021 10:24
Copy link
Member

@shakuzen shakuzen left a comment

Choose a reason for hiding this comment

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

Thank you for the pull request.
As mentioned in the linked issue, I think backends other than Datadog do not support what you're wanting to do. So allowing something like this in micrometer-core is likely to cause problems unless you're only publishing metrics to Datadog. So I'm not sure yet that this is the best approach.

@datibbaw
Copy link
Author

The idea behind this approach is that existing code will continue to work in the same way, i.e. tags are deduplicated based on their keys; developers are required to override the Tag#compareTo() method with a class of their own, as a means to signal that they understand the consequences of their actions.

But maybe I've missed something; this is a big codebase, and I don't assume to know a whole lot about it other than attempting to scratch an itch for our own projects.

@datibbaw datibbaw force-pushed the datibbaw/tag-dedup-patch branch from 23a909a to 52bd728 Compare August 29, 2021 03:45
@datibbaw datibbaw requested a review from shakuzen September 4, 2021 09:01
@datibbaw
Copy link
Author

@shakuzen Could you have another look at this? The code change itself doesn't do anything new, rather it uses the existing Tag interface to accomplish the same functionality while opening the implementation up for extension where necessary. I've added an example of this in the tests section.

@datibbaw
Copy link
Author

@shakuzen Hoping to get an answer to my earlier question; even though we use only Datadog in our organisation, it's the abstraction of metrics that we like to keep using.

@shakuzen
Copy link
Member

Sorry for the lack of response. I'll take a closer look at this again next week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants