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 self signed ssl to test server and test netty client #2344

Merged
merged 1 commit into from
Jan 29, 2021

Conversation

tylerbenson
Copy link
Contributor

Update netty client instrumentation to accurately capture if https.

@tylerbenson tylerbenson requested a review from a team as a code owner January 28, 2021 17:34
@tylerbenson tylerbenson marked this pull request as draft January 28, 2021 17:34
@tylerbenson tylerbenson force-pushed the tyler/netty-client-https-testing branch 6 times, most recently from 401322a to ec2130c Compare January 28, 2021 22:38
Update netty client instrumentation to accurately capture if https.
@tylerbenson tylerbenson force-pushed the tyler/netty-client-https-testing branch from ec2130c to 0743bdc Compare January 28, 2021 23:02
@@ -39,13 +40,15 @@ public void write(final ChannelHandlerContext ctx, final Object msg, final Chann
final HttpRequest request = (HttpRequest) msg;

ctx.channel().attr(AttributeKeys.CLIENT_PARENT_ATTRIBUTE_KEY).set(activeSpan());
boolean isSecure = ctx.pipeline().get("ssl") != null;
NettyHttpClientDecorator decorate = isSecure ? DECORATE_SECURE : DECORATE;
Copy link
Member

Choose a reason for hiding this comment

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

Can we make this a noun not a verb? I've always found this convention a bit strange to read

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd prefer to keep it consistent for now. If we want to change it, that should be done everywhere.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure, changing the names in one go is a cross cutting (i.e. disruptive) change. This is a new local variable name and doesn't need to be consistent with anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you let me know what you'd prefer, I'll' make a PR that changes it everywhere.

Copy link
Member

@richardstartin richardstartin left a comment

Choose a reason for hiding this comment

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

👍🏻

@tylerbenson tylerbenson marked this pull request as ready for review January 29, 2021 15:35
@tylerbenson tylerbenson merged commit c5076ec into master Jan 29, 2021
@tylerbenson tylerbenson deleted the tyler/netty-client-https-testing branch January 29, 2021 18:12
@github-actions github-actions bot added this to the 0.73.0 milestone Jan 29, 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.

None yet

3 participants