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

Add connector name tag for JettyConnectionMetrics and JettySslHandshakeMetricsBinder #2603

Merged
merged 1 commit into from
Jul 21, 2021

Conversation

izeye
Copy link
Contributor

@izeye izeye commented May 12, 2021

This PR adds connector name tag for JettyConnectionMetrics.

See spring-projects/spring-boot#26418 (comment)

/cc @wilkinsona

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.

I'm generally in favor of this change, but it makes an asymmetry between the outcome of the two ways the JettyConnectionMetrics can be applied. If you do connector.addBean(new JettyConnectionMetrics(registry)) then the connector.name tag won't be there. Since we don't have a reference to the Connector, we can't do much about that given the current API. I wonder if we shouldn't move the API in the direction of passing the Connector to the constructor so we can add the connector.name tag in either case.

@shakuzen shakuzen added this to the 1.8 backlog milestone May 13, 2021
@shakuzen shakuzen added enhancement A general enhancement module: micrometer-core An issue that is related to our core module labels May 13, 2021
@wilkinsona
Copy link
Contributor

I also wonder if it should be possible not to use the connector’s name as a tag? In the

@izeye
Copy link
Contributor Author

izeye commented May 13, 2021

@wilkinsona I'm not sure if I fully understand your comment. Do you mean a flag to control addition of the tag for connector's name? Maybe, you might mean to provide more context in the following incomplete sentence that seems to have been removed accidentally.

@wilkinsona
Copy link
Contributor

Oops. Sorry, not sure how I managed that. I intended to say the following:

In the single connector case, if things change as suggested above by @shakuzen, that could be done by only setting the name tag if the user just passes in the Connector. If they call a Connector, Iterable<Tag> method then the default name tag could perhaps be skipped. That doesn't work so well for the multi-connector case as there's no way to provide connector-specific tags. Maybe that doesn't matter so much and the user could just call the new single connector method multiple times?

@izeye
Copy link
Contributor Author

izeye commented May 13, 2021

@wilkinsona Thanks for clarification! I can see how it could work, but I'm not sure if it's natural to use them as a signal for adding the tag. Maybe, I might miss something.

@izeye izeye force-pushed the jetty-connector-name-tag branch 2 times, most recently from b25752c to ac2c3a5 Compare July 20, 2021 15:06
@izeye
Copy link
Contributor Author

izeye commented Jul 20, 2021

I updated this PR to try to apply the suggestion from @shakuzen. I didn't deprecate the previous constructors as they provide a way not to use the connector's name as a tag. I wasn't able to apply the suggestion from @wilkinsona as I don't think I fully understand it.

Let me know what you think.

If it's okay, I'll update this PR to apply the same changes to JettySslHandshakeMetricsBinder as well.

See spring-projects/spring-boot#26418 (comment)

@wilkinsona
Copy link
Contributor

wilkinsona commented Jul 20, 2021

Sorry for the confusion, @izeye. Please ignore my suggested way of achieving the goal. It's really just the goal itself that's important and that is for a user to be able to register metrics for a Connector without the connector.name tag being added automatically. If this isn't a pattern that's followed elsewhere in Micrometer then please disregard this. I'd rather that the connector.name tag was always added than it never was.

@izeye izeye force-pushed the jetty-connector-name-tag branch from ac2c3a5 to 45aec8b Compare July 20, 2021 17:00
@izeye
Copy link
Contributor Author

izeye commented Jul 20, 2021

@wilkinsona Thanks for the feedback!

I updated this PR to apply the same changes to JettySslHandshakeMetricsBinder.

Please let me know if anything else needs to be done.

@wilkinsona
Copy link
Contributor

Looks good to me, @izeye.

@izeye izeye changed the title Add connector name tag for JettyConnectionMetrics Add connector name tag for JettyConnectionMetrics and JettySslHandshakeMetricsBinder Jul 20, 2021
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.

Looks good to me.

@shakuzen shakuzen merged commit 82a8586 into micrometer-metrics:main Jul 21, 2021
@shakuzen shakuzen modified the milestones: 1.8 tentative, 1.8.0-M2 Jul 21, 2021
shakuzen added a commit that referenced this pull request Jul 21, 2021
@izeye izeye deleted the jetty-connector-name-tag branch July 21, 2021 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A general enhancement module: micrometer-core An issue that is related to our core module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants