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

Set http.route in spring-autoconfigure webmvc instrumentation #6414

Merged

Conversation

mateuszrzeszutek
Copy link
Member

Resolves #3479

I think this feature should be integrated into the spring-webmvc library instrumentation; it'd require bumping the minimum version to 5.3 though. I think we should do that, having the route is super useful, and the webmvc library instrumentation is probably only used in autoconfigure anyway.

@mateuszrzeszutek mateuszrzeszutek requested a review from a team August 3, 2022 08:18
@trask
Copy link
Member

trask commented Aug 4, 2022

I think this feature should be integrated into the spring-webmvc library instrumentation; it'd require bumping the minimum version to 5.3 though. I think we should do that, having the route is super useful, and the webmvc library instrumentation is probably only used in autoconfigure anyway.

👍 to bumping the min version for spring-webmvc library instrumentation

do you want to merge this PR first, or make that change here?

@mateuszrzeszutek
Copy link
Member Author

I could do this in this PR, I don't mind either way

Comment on lines +72 to +80
default Integer statusCode(REQUEST request, RESPONSE response, @Nullable Throwable error) {
return statusCode(request, response);
}
Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted the Spring/Servlet status code to be properly reflected in the span status, so I added this method. I'll deprecate the previous one in the next PR.

* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.instrumentation.spring.webmvc.v5_3;
Copy link
Member Author

Choose a reason for hiding this comment

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

I also changed the package name, in accordance with #932

Copy link
Member

Choose a reason for hiding this comment

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

I had to think through this (#932 (comment)), but I agree

import org.junit.jupiter.api.extension.RegisterExtension;
import org.springframework.context.ConfigurableApplicationContext;

class WebMvcHttpServerTest extends AbstractHttpServerTest<ConfigurableApplicationContext> {
Copy link
Member

Choose a reason for hiding this comment

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

nice!

Comment on lines +54 to +57
// we can't retrieve the handler mappings from the DispatcherServlet in the onRefresh listener,
// because it loads them just after the application context refreshed event is processed
// to work around this, we're setting a boolean flag that'll cause this filter to load the
// mappings the next time it attempts to set the http.route
final class WebContextRefreshListener implements ApplicationListener<ContextRefreshedEvent> {
Copy link
Member

Choose a reason for hiding this comment

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

just curious if this gets hit in the integration tests (I'm ok if not)

Copy link
Member Author

Choose a reason for hiding this comment

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

It does; it has to be hit if the http.route is to be present in generated spans.

@mateuszrzeszutek mateuszrzeszutek force-pushed the spring-autoconfigure-http-route branch from f76c42f to 1fa1664 Compare August 9, 2022 12:45
@trask trask merged commit 0d1cc8e into open-telemetry:main Aug 9, 2022
@mateuszrzeszutek mateuszrzeszutek deleted the spring-autoconfigure-http-route branch August 9, 2022 17:01
LironKS pushed a commit to helios/opentelemetry-java-instrumentation that referenced this pull request Oct 23, 2022
…elemetry#6414)

* Set http.route in spring-autoconfigure webmvc instrumentation

* Bump spring-webmvc library instrumentation version to 5.3

* nit: protected -> private

* Remove duplicated test (already covered by HttpSpanStatusExtractorTest)

* Move the README to the correct module

* fix link

* fix more links

* liiiiiiinks

* fix tests

* remove not needed weakref
LironKS pushed a commit to helios/opentelemetry-java-instrumentation that referenced this pull request Oct 31, 2022
…elemetry#6414)

* Set http.route in spring-autoconfigure webmvc instrumentation

* Bump spring-webmvc library instrumentation version to 5.3

* nit: protected -> private

* Remove duplicated test (already covered by HttpSpanStatusExtractorTest)

* Move the README to the correct module

* fix link

* fix more links

* liiiiiiinks

* fix tests

* remove not needed weakref
LironKS pushed a commit to helios/opentelemetry-java-instrumentation that referenced this pull request Dec 4, 2022
…elemetry#6414)

* Set http.route in spring-autoconfigure webmvc instrumentation

* Bump spring-webmvc library instrumentation version to 5.3

* nit: protected -> private

* Remove duplicated test (already covered by HttpSpanStatusExtractorTest)

* Move the README to the correct module

* fix link

* fix more links

* liiiiiiinks

* fix tests

* remove not needed weakref
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.

Spring WebMVC spanName is contant
2 participants