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

Review semantic convention for Http Client spans #1214

Merged
merged 5 commits into from
Sep 18, 2020

Conversation

iNikem
Copy link
Contributor

@iNikem iNikem commented Sep 17, 2020

No description provided.

Comment on lines -100 to -101
public static final boolean DEFAULT_LOG_INJECTION_ENABLED = false;
public static final String DEFAULT_EXPERIMENTAL_LOG_CAPTURE_THRESHOLD = null;
Copy link
Member

Choose a reason for hiding this comment

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

👍

Comment on lines -79 to -80
public static final String HTTP_SERVER_TAG_QUERY_STRING = "http.server.tag.query-string";
public static final String HTTP_CLIENT_TAG_QUERY_STRING = "http.client.tag.query-string";
Copy link
Member

Choose a reason for hiding this comment

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

👍

@@ -52,4 +55,45 @@ abstract class JdkHttpClientTest extends HttpClientTest {
boolean testCircularRedirects() {
return false
}

//We override this test below because it produces somewhat different attributes
Copy link
Member

Choose a reason for hiding this comment

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

is it only flavor that's different?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

Comment on lines +82 to +83
SemanticAttributes.HTTP_FLAVOR.set(
span, httpResponse.version() == Version.HTTP_1_1 ? "1.1" : "2.0");
Copy link
Member

Choose a reason for hiding this comment

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

interesting, makes sense for a negotiated connection, where we don't know flavor ahead of time

@@ -46,6 +47,11 @@ protected String method(HttpRequest request) {
}
}

@Override
protected @Nullable String flavor(HttpRequest httpRequest) {
Copy link
Member

Choose a reason for hiding this comment

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

I like this placement (type annotation) 👍

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 don't :) It is IDEA's doing, so I have to accept :(

Copy link
Contributor

Choose a reason for hiding this comment

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

Reformat code puts the annotation there? I haven't seen that before. I agree with @iNikem that for normal methods, annotation above looks a bit better mostly due to how annotations end up getting colored in IDE GitHub etc. @iNikem Is it not worth manually fixing these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not reformat but "implement missing method", I think.

I don't want to fix this manually every time IDEA generates code for me. I can do it here, but it will not be sustainable

Copy link
Member

Choose a reason for hiding this comment

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

ya, i like type annotations, but they are not common. i'm very surprised Intellij does the "right thing" 😉

@iNikem iNikem merged commit beb1901 into open-telemetry:master Sep 18, 2020
@iNikem iNikem deleted the http-client-convention branch September 18, 2020 14:52
@trask trask mentioned this pull request Oct 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.

3 participants