Skip to content

Upgrade to Micrometer 1.0.0-SNAPSHOT #11776

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.

Sign up for GitHub

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

Jump to bottom
Closed
jkschneider wants to merge 9 commits into from
Closed

Upgrade to Micrometer 1.0.0-SNAPSHOT #11776

jkschneider wants to merge 9 commits into from

Conversation

@jkschneider
Copy link
Contributor

@jkschneider jkschneider commented Jan 25, 2018 •
edited
Loading

  • We now have Jersey 2 support with the module micrometer-jersey2. It's not clear to me how we could register MetricsApplicationEventListener for Jersey 2 based Boot apps.
  • I think the servlet filter is generic enough now to call it MetricsFilter rather than WebMvcMetricsFilter. At any rate, the goal is to make it not WebMvc specific.

The following issues are addressed by this PR:

@jkschneider
Initial upgrade to Micrometer 1.0.0-SNAPSHOT
2fedd09 
* Partial integration of null safety from Micrometer spring-projects#310
* Improved assertions on meter vlaues from Micrometer spring-projects#261
* Servlet filter restructuring due to generalization coming out of Jersey2 support in Micrometer spring-projects#228
* Fixed async servlet handling as part of Micrometer spring-projects#228
@jkschneider jkschneider force-pushed the micrometer-1.0.0-SNAP branch from 6466ca1 to 2fedd09 Compare January 25, 2018 18:52
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jan 25, 2018
@jkschneider jkschneider mentioned this pull request Jan 25, 2018
Closed
@jkschneider jkschneider mentioned this pull request Jan 25, 2018
Closed
@philwebb philwebb added priority: high type: dependency-upgrade A dependency upgrade and removed status: waiting-for-triage An issue we've not yet triaged labels Jan 25, 2018
@philwebb philwebb added this to the 2.0.0.RC1 milestone Jan 25, 2018
@philwebb philwebb self-assigned this Jan 25, 2018
@philwebb philwebb added the for: merge-with-amendments Needs some changes when we merge label Jan 26, 2018
@philwebb
Copy link
Member

philwebb commented Jan 26, 2018

I'm having a bit of a difficult time getting this one into a state that we can merge. I've so jar just been working on the first commit in the set. I've split it apart and tried to create smaller commits with related concerns. I've rename the MVC classes back to their old names (they are still very tied to MVC so I don't like the very generic ServletMetricsFilter name.

My branch is here.

Things go relatively OK until the last commit. The changes to WebMvcMetricsFilter are a big concern to me. I've added some FIXME comments in the code but the main worry is around the asyncTimingContext and the fact that it's a synchronized map.

@jkschneider
Copy link
Contributor Author

jkschneider commented Jan 26, 2018

@philwebb Good idea on the request attribute. Tested it against micrometer-spring-legacy successfully and committed there: micrometer-metrics/micrometer@44a5d88#diff-974dd55a85a31feb0d4a829577d72046d

@wilkinsona wilkinsona mentioned this pull request Jan 26, 2018
Closed
@ITman1
Copy link

ITman1 commented Jan 26, 2018

I suggest to add @ConditionalOnMissingBean on every XxxxxxxMeterRegistry bean auto-configuration. I know there is currently @ConditionalOnProperty("management.metrics.export.xxxxxxx.enabled"), which might be enough for application using spring-boot directly, but not for libraries which needs some customizations without affecting properties.

@philwebb
Copy link
Member

philwebb commented Jan 26, 2018 •
edited
Loading

Sorry but there is just way too much going on in the PR for me to be able to merge it before RC1. I've split out the following issues which we can prioritize and deal with on a case-by-case basis:

#11348
#11797
#11798
#11799
#11800
#11801
#11802
#11803
#11804

@philwebb philwebb closed this Jan 26, 2018
@philwebb philwebb added status: declined A suggestion or change that we don't feel we should currently apply and removed for: merge-with-amendments Needs some changes when we merge priority: high type: dependency-upgrade A dependency upgrade labels Jan 26, 2018
@philwebb philwebb removed this from the 2.0.0.RC1 milestone Jan 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@philwebb philwebb

Labels

status: declined A suggestion or change that we don't feel we should currently apply

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@jkschneider @philwebb @ITman1 @spring-projects-issues @GreyTeardrop