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

Align tag and metric names with Micrometer’s recommendations #30536

Closed
wilkinsona opened this issue Apr 5, 2022 · 4 comments
Closed

Align tag and metric names with Micrometer’s recommendations #30536

wilkinsona opened this issue Apr 5, 2022 · 4 comments
Assignees
Labels
type: enhancement A general enhancement
Milestone

Comments

@wilkinsona
Copy link
Member

wilkinsona commented Apr 5, 2022

Micrometer recommends that metric and tag names are lower-case and dot-separated. We should review our default metrics and tags to identify those that do not comply with the recommendations. Once we know how many non-compliant names we’re dealing with, we can consider how and when to change them.

Tentatively scheduling for 2.7 but this may get pushed back.

@wilkinsona
Copy link
Member Author

The following are non-compliant tag names:

return (mainClass != null) ? this.tags.and("main-application-class", mainClass.getName()) : this.tags;

I haven't found any non-compliant metric names.

The three GraphQL-related tags are all new in 2.7 so those can be changed without much further thought. The others were added in 2.6 or earlier so renaming them may have an impact on production dashboards. There are a few different options available to us:

  • Rename them and add an entry to the release notes describing how to implement MeterFilter and map(Id) to restore the old names if necessary
  • Add an equivalent lower-case dot-separated tag alongside the current tag in 2.7 (or even a maintenance release) to provide a migration period before removing the non-compliant tags in 3.0
  • Leave them as-is and recommend the use of MeterFilter and map(Id) to change them if necessary

I'm going to ask the Observability team for some advice on how best to handle this.

@checketts
Copy link
Contributor

Camelcase seems fine. I think the problem with hyphens is they can be interpreted as subtraction in some contexts (like a promql query).

@wilkinsona
Copy link
Member Author

From @shakuzen:

We have some in Micrometer instrumentation we need to consider what to do with ourselves. Sorry we didn't find these and discuss earlier. We'll discuss internally to come up with general guidance on this and plan to document it better.
It's not entirely a problem what Boot has now, though. Metric names tend to have a hierarchy to them that is separated, so it is important there. Tag names on metrics tend to have less firm convention in metrics backends and less/no hierarchy to them.

From @jonatan-ivanov:

As @checketts pointed out, camelCase should be ok. It won’t be handled as separated words but it should not break anything. So you can leave them as-is if you want to narrow the scope and only fix main-application-class.

I might be a little draconian here but I would go with the first option: making the change and describe the way to restore the old value using a MeterFilter.

@wilkinsona wilkinsona added the for: team-meeting An issue we'd like to discuss as a team to make progress label Apr 6, 2022
@wilkinsona
Copy link
Member Author

We're going to change all the incompatible tag names to the recommended lower.case format in 2.7.

@wilkinsona wilkinsona removed the for: team-meeting An issue we'd like to discuss as a team to make progress label Apr 6, 2022
@wilkinsona wilkinsona self-assigned this Apr 11, 2022
@wilkinsona wilkinsona modified the milestones: 2.7.x, 2.7.0-RC1 Apr 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

2 participants