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

Instrument spring-web 6 & spring-webmvc 6 #7366

Merged
merged 3 commits into from
Dec 12, 2022

Conversation

mateuszrzeszutek
Copy link
Member

Part of #7203

This PR is mostly copy-paste and working around the differences, conceptually the new instrumentation is the same as the old one

@mateuszrzeszutek mateuszrzeszutek requested a review from a team December 8, 2022 12:05
@trask trask added this to the v1.21.0 milestone Dec 8, 2022
Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

do you think we should port ServletFilterTest to 6.0 at some point?

if (springCtx.containsBean("otelAutoDispatcherFilter")) {
OpenTelemetryHandlerMappingFilter filter =
(OpenTelemetryHandlerMappingFilter) springCtx.getBean("otelAutoDispatcherFilter");
if (handlerMappings != null) {
Copy link
Member

Choose a reason for hiding this comment

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

what was reason for not keeping the filter != null condition?

Copy link
Member Author

Choose a reason for hiding this comment

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

getBean() will never return null - it'll either return the filter or throw.

@mateuszrzeszutek
Copy link
Member Author

do you think we should port ServletFilterTest to 6.0 at some point?

Yeah, we should. I didn't want to do it in this PR, as it's already quite huge. I'll add the test in one of the next PRs.

@trask trask merged commit 59b7513 into open-telemetry:main Dec 12, 2022
@mateuszrzeszutek mateuszrzeszutek deleted the instrument-spring-web-6 branch December 12, 2022 18:09
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.

3 participants