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 library instrumentation for Ratpack server #3749

Merged
merged 9 commits into from
Aug 4, 2021

Conversation

anuraaga
Copy link
Contributor

@anuraaga anuraaga commented Aug 2, 2021

The attributes are not very complete yet, I will try to follow up to do so. One major annoyance is e.g., accessing remoteAddress involves an unstable Guava type that has had its API changed throughout the versions... On the bright side the tests are shareable.

Ideally we can use this in the javaagent instrumentation I guess, but we have our tricky chicken / egg of Netty instrumentation currently winning over any framework-specific server instrumentation if on the same thread.

On the bright side, might be able to replace the execution-related instrumentation with the library instrumentation pattern.

Will add client in the future.

response.contentUtf8() == SUCCESS.body

assertTraces(1) {
trace(0, 2 + (hasHandlerSpan(SUCCESS) ? 1 : 0)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only difference in test logic is this sort of hasHandlerSpan branch


final class RatpackGetter implements TextMapGetter<Request> {

RatpackGetter() {}
Copy link
Member

Choose a reason for hiding this comment

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

Nit: that constructor is unnecessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIUC a public method in a private class is still accessible with reflection, there is no Class.setAccessible a private class is respected by javac but has no effect at runtime I think. So to ensure privateness of the constructor it has to be made private.

This of course assumes the constructor of a private class is public like public classes I assumed it is but am not sure.


static final RatpackProductionErrorHandler INSTANCE = new RatpackProductionErrorHandler();

private static final Logger logger = LoggerFactory.getLogger(RatpackProductionErrorHandler.class);
Copy link
Member

Choose a reason for hiding this comment

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

This is using "our" slf4j, not ratpack's slf4j. Is that okay?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm good point. In practice I don't think it's possible to use this class as the context should always have a error handler I think. Should I verify that and throw an exception outside of the loop that finds delegate instead of this fallback? Maybe slightly dangerous if there is some unexpected ratpack behavior though.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, maybe just logging something would be enough? And a bit safer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a WarnOnce

anuraaga and others added 3 commits August 3, 2021 18:13
…etry/instrumentation/ratpack/RatpackTracingBuilder.java

Co-authored-by: Mateusz Rzeszutek <[email protected]>
@anuraaga anuraaga merged commit e92ecc0 into open-telemetry:main Aug 4, 2021
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.

2 participants