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 data streams monitoring instrumentation for HTTP #4266

Merged
merged 18 commits into from
Dec 1, 2022
Merged

Conversation

hokitam
Copy link
Contributor

@hokitam hokitam commented Nov 16, 2022

What Does This Do

This change adds data streams monitoring instrumentation for HTTP client and server libraries.

For HTTP server libraries, we are able to make the change directly in a single location: in HttpServerDecorator, where the trace context extract() call is made today. To avoid changing the method signature of extract(), we make our call in startSpan instead.

For HTTP client libraries, we add an additional injectPathwayContext() call wherever we make an inject() call today.

Motivation

Support HTTP for Data Streams Monitoring.

Additional Notes

This change was tested manually with demo apps using Apache HTTP client and Java Servlet deployed via Tomcat. Unit tests were added via HttpClientTest and HttpServerTest, which are used as the base tests for the HTTP libraries that the Java tracer supports.

@hokitam hokitam marked this pull request as ready for review November 21, 2022 20:02
@hokitam hokitam requested a review from a team as a code owner November 21, 2022 20:02
@hokitam hokitam changed the title Add DSM instrumentation for HTTP Add data streams monitoring instrumentation for HTTP Nov 21, 2022
Copy link
Contributor

@mcculls mcculls left a comment

Choose a reason for hiding this comment

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

General comment: it's unfortunate that injecting the pathway context couldn't have been made part of the existing propagate().inject(...) call because that would have meant fewer changes to the codebase.

Even a method that did both such as propagate().injectWithPathway(span, request, SETTER, pathwayTags) would avoid having all these extra method calls per-request.

Copy link
Contributor

@mcculls mcculls left a comment

Choose a reason for hiding this comment

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

Looks ok, given the limitations of the current pathway context API

(there appears to be a lot of opportunity in that code to revisit the API and reduce overhead)

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