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

Ratpack httpclient #4787

Merged
merged 15 commits into from
Jan 18, 2022
Merged

Ratpack httpclient #4787

merged 15 commits into from
Jan 18, 2022

Conversation

jsalinaspolo
Copy link
Contributor

Proposal to support Ratpack HttpClient Instrumentation. The intention of the PR is to start a conversation about the proposal

🎉 Ratpack 1.7.0 supports HttpClient request/response/error interceptors, therefore ratpack version have been upgraded

  • Does Ratpack HttpClient needs ContextPropagation, I have been unable to see the need but I am probably missing something.
  • Should RatpackTracingBuilder support RatpackHttpTracing or should be treated independently

@anuraaga
Copy link
Contributor

anuraaga commented Dec 3, 2021

Thanks #4787 - I think bumping the version makes sense, 1.7 is pretty old and I haven't heard any demand for supporting older versions. Do you mind sending an initial PR with just the version bump to make these easier to review?

@jsalinaspolo jsalinaspolo force-pushed the ratpack-httpclient branch 2 times, most recently from 13400cf to 10261ac Compare December 3, 2021 08:33
@jsalinaspolo
Copy link
Contributor Author

Better to leave the renaming of the module for after the review 🙇

@iNikem
Copy link
Contributor

iNikem commented Dec 3, 2021

As I understand this introduces new library instrumentation for Ratpack and this requires v1.7. Do we actually need to upgrade our server instrumentation to 1.7 as well or we can leave it to continue to work with 1.4?

@jsalinaspolo
Copy link
Contributor Author

We can create a new module to ratpack-1.7 and leave the ratpack-1.4 as it is, but I am not sure if there is any need to support ratpack-1.4 that is over 4 years old. Something I have been thinking is if we should be validating that the instrumentation works with newer version as well. What is the current approach for other library instrumentation?

@laurit
Copy link
Contributor

laurit commented Dec 3, 2021

@jsalinaspolo

Something I have been thinking is if we should be validating that the instrumentation works with newer version as well. What is the current approach for other library instrumentation?

When you run tests with-PtestLatestDeps=true the version used in library and testLibrary dependencies is automatically changed to latest available version. If the latest available version doesn't work for some reason you can use latestDepTestLibrary. Tests against latest version are run on merge to main and during nightly builds.

@jsalinaspolo
Copy link
Contributor Author

Thanks #4787 - I think bumping the version makes sense, 1.7 is pretty old and I haven't heard any demand for supporting older versions. Do you mind sending an initial PR with just the version bump to make these easier to review?

Sorry @anuraaga, I misunderstood you and undo the renaming of the module so the PR is easier to review. Do you think that makes sense to keep this PR to discuss the instrumentation approach of the Ratpack HttpClient and once it is reviewed, we can rename the module to 1.7?

@iNikem the instrumentation of the Ratpack HttpClient requires interceptors, therefore we need version 1.7. If you think that we need to keep version 1.4, then we could create a new module with ratpack-1.7 with the HttpClient instrumentation. Personally, I think would be better to bump to 1.7 altogether, anyway 1.7 is over 2 years old

@anuraaga
Copy link
Contributor

anuraaga commented Dec 4, 2021

I was hoping to do the version bump first, including directory rename, if there is consensus for it. It's a bit tricky discerning changes made for the bump vs ones for client instrumentation.

We can create a new module to ratpack-1.7 and leave the ratpack-1.4 as it is, but I am not sure if there is any need to support ratpack-1.4 that is over 4 years old.

My vote is to bump for now - that being said I think we need a proper process for sunsetting instrumentation, which we haven't really done before. Perhaps we should target the next release for this change, first updating current instrumentation to log a warning that the version floor will change. Anyone have any thoughts?

@jsalinaspolo jsalinaspolo mentioned this pull request Dec 4, 2021
@jsalinaspolo jsalinaspolo force-pushed the ratpack-httpclient branch 2 times, most recently from db4aceb to d013857 Compare December 7, 2021 15:53
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.

Can you add http client library tests that extends AbstractRatpackHttpClientTest? or at least tests that extends AbstractHttpClientTest if the AbstractRatpackHttpClientTest is not appropriate for library instrumentation? the base http client tests run a large array of standard tests to ensure that all of our http client instrumentations generate consistent telemetry.

@jsalinaspolo
Copy link
Contributor Author

jsalinaspolo commented Dec 29, 2021

Can you add http client library tests that extends AbstractRatpackHttpClientTest? or at least tests that extends AbstractHttpClientTest if the AbstractRatpackHttpClientTest is not appropriate for library instrumentation? the base http client tests run a large array of standard tests to ensure that all of our http client instrumentations generate consistent telemetry.

I've been digging into the AbstractHttpClientTest and figuring out how it is working. I've added context propagation to the HttpClient but still, a few tests are failing. Most of them are related to having more traces than expected, but I am not sure if it is related to suppressing nested CLIENT spans or something else.

Span span = Span.fromContext(otelCtx);
String path = requestSpec.getUri().getPath();
span.updateName(path);
// span.updateName("HTTP " + requestSpec.getMethod().getName()); // TODO use path instead of [HTTP method]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on the spec should be possible to leave name like /api/users/{user_id} instead of HTTP GET ?

Copy link
Member

Choose a reason for hiding this comment

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

it depends on if path is a template (e.g. /api/users/{user_id}) or if it's a real url path (e.g. /api/users/5551234)

@trask
Copy link
Member

trask commented Jan 5, 2022

I've been digging into the AbstractHttpClientTest and figuring out how it is working. I've added context propagation to the HttpClient but still, a few tests are failing. Most of them are related to having more traces than expected, but I am not sure if it is related to suppressing nested CLIENT spans or something else.

thx @jsalinaspolo! i'll have a look at the failing tests tomorrow 👍

@trask
Copy link
Member

trask commented Jan 6, 2022

hey @jsalinaspolo, thanks for your patience. the problem in the tests appears to be that they are losing (local) context propagation across threads, somewhere between where the (synthetic) "parent" span is created and where the http client span is created.

the reason the same tests probably work in the javaagent tests is because the javaagent instruments java executors in order to propagate context across threads

I think your other library test InstrumentedHttpClientTest is working because you are using RatpackTracing.create(openTelemetry) over there which looks like it registers OpenTelemetryExecInterceptor.INSTANCE to take care of context propagation for ratpack threads.

@jsalinaspolo
Copy link
Contributor Author

jsalinaspolo commented Jan 9, 2022

Thanks @trask for looking into it and the hint.

Finally, it was a bit simpler than I thought 😌 . Only was required to pass the current Context to the test execution like here

@trask
Copy link
Member

trask commented Jan 10, 2022

@jsalinaspolo thanks for working through the tests! there's one more test issue, can you run this locally and check the failure?

./gradlew :instrumentation:ratpack:ratpack-1.7:library:test -PtestLatestDeps=true

testLatestDeps will run the tests against the latest version of ratpack in maven central (there are ways to configure this if there's a reason we intentionally don't support the latest version yet).

sometimes failures against the latest dependency point to issues in the instrumentation, and sometimes issues with the tests themselves. if it's not clear how to make the instrumentation and tests work against both the base version and latest version, go ahead and post your findings, and we can help you sort it out (we have a variety of ways to deal with this common issue)

@jsalinaspolo
Copy link
Contributor Author

jsalinaspolo commented Jan 11, 2022

@trask looks like was an issue in the functional test how was starting the otherApp.
Should be fix now by this 🙇

import ratpack.http.client.RequestSpec;

/** Entrypoint for tracing OkHttp clients. */
public final class RatpackHttpTracing {
Copy link
Contributor

Choose a reason for hiding this comment

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

Because Ratpack HTTP client can really only be used in a Ratpack server, do you think it makes sense to just keep one package and combine the entrypoints into one? Notably configureServerRegistry can add the exec initializer too to keep things simpler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as we are happy to add the exec initializer at all times, I think might be easier to configure.

The only downside would be having too many classes in a package without structure while having client server and internal might help to clarify.

Having different packages with a single RatpackTracing have the problem that a few classes would need to be public, which I understood was not ideal.

What do you think would be nicer? @anuraaga

Copy link
Contributor Author

@jsalinaspolo jsalinaspolo Jan 15, 2022

Choose a reason for hiding this comment

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

I made a refactor to have a flatter package structure following your suggestion @anuraaga

@jsalinaspolo
Copy link
Contributor Author

@anuraaga @trask is the PR good to go?

Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

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

Just small point thanks!

}

/** Returns instrumented instance of {@link HttpClient} with OpenTelemetry. */
public HttpClient instrumentedHttpClient(HttpClient httpClient) throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's name it instrumentHttpClient

@trask trask merged commit 95e240c into open-telemetry:main Jan 18, 2022
@trask
Copy link
Member

trask commented Jan 18, 2022

thx @jsalinaspolo!

@jsalinaspolo jsalinaspolo deleted the ratpack-httpclient branch January 19, 2022 20:22
RashmiRam pushed a commit to RashmiRam/opentelemetry-auto-instr-java that referenced this pull request May 23, 2022
* Add Ratpack HttpClient instrumentation

* Propagate trace through Ratpack HttpClient

* Add test to verify trace propagation

* Fix spotlessApply

* Use HTTP method as name for ratpack http client

* Add current Context to the execution

* Fix HttpClient tests

* Move Ratpack HttpClient tests to java package

* Remove nullaway conventions from library

* Add Context to ExecHarness executions

* Remove ContextHolder from execution

* Fix function test using other server stub

* Fix lazy other app

* Refactor ratpack client packages

* Rename getter method
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.

5 participants