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

Populate Zipkin remoteEndpoint #4933

Merged
merged 5 commits into from
Nov 28, 2022

Conversation

jonatan-ivanov
Copy link
Member

fixes gh-4932

@jonatan-ivanov jonatan-ivanov requested a review from a team November 10, 2022 21:45
@codecov
Copy link

codecov bot commented Nov 10, 2022

Codecov Report

Base: 91.00% // Head: 91.06% // Increases project coverage by +0.05% 🎉

Coverage data is based on head (505a7da) compared to base (cc1d6ae).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #4933      +/-   ##
============================================
+ Coverage     91.00%   91.06%   +0.05%     
+ Complexity     4840     4810      -30     
============================================
  Files           546      544       -2     
  Lines         14447    14317     -130     
  Branches       1395     1369      -26     
============================================
- Hits          13148    13038     -110     
+ Misses          893      882      -11     
+ Partials        406      397       -9     
Impacted Files Coverage Δ
...y/exporter/zipkin/OtelToZipkinSpanTransformer.java 93.02% <100.00%> (+1.13%) ⬆️
...opentelemetry/extension/aws/AwsXrayPropagator.java
...metry/extension/aws/AwsConfigurablePropagator.java
...y/exporter/internal/marshal/CodedOutputStream.java 71.00% <0.00%> (+1.18%) ⬆️
...metry/sdk/metrics/export/PeriodicMetricReader.java 90.00% <0.00%> (+2.85%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@breedx-splk breedx-splk left a comment

Choose a reason for hiding this comment

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

This looks good to me. I had a small question about possibly filtering out the remote endpoint attributes as redundant. Curious what other folks think.

Comment on lines +192 to +195
.remoteEndpoint(expectedRemoteEndpoint)
.putTag(PEER_SERVICE.getKey(), "remote-test-service")
.putTag(NET_SOCK_PEER_ADDR.getKey(), "8.8.8.8")
.putTag(NET_PEER_PORT.getKey(), "42")
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a bummer that these are kinda redundant (duplicated) now -- they exist both in the remoteEndpoint and in the tags. Should we filter these specific keys out of the tags when mapping the attributes over?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fyi: there will be lots of things to filter out: https://opentelemetry.io/docs/reference/specification/trace/sdk_exporters/zipkin/#otlp---zipkin

What happens in this class seems wrong to me (but I guess now it's too late for that); I think the span (SpanData?) should have these as separate fields not as attributes. I'm biased though: I prefer having type-safe fields instead of pouring everything into a "hashmap".

Also, just an fyi: as a user of OTel, I would consider this as a breaking change (a tag will be removed from the span). I'm not sure if doing such a change is even allowed here (maybe it is since semconv is unstable so deleting/renaming attributes and breaking users that way is possible at this point here).
It might worth to check other exporters too, i.e.: will those tags be shipped to Jaeger/OTLP/etc?

Because of these, I think deleting (filtering out) those attributes should happen in a separate issue/pr.

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 biased though: I prefer having type-safe fields instead of pouring everything into a "hashmap".

I assume the decision to model fields in map-like structure instead of in top level fields is because it affords more ability to experiment without the consequences of adding a top level field we later wish to remove / rename / or change. This relieves bikeshedding (to some extent anyway 😄).

Additionally, since spans can potentially be used to model many many domains, the top level fields could easily get bogged down as more and more top level fields were added.

Copy link
Member Author

Choose a reason for hiding this comment

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

without the consequences of adding a top level field we later wish to remove / rename / or change

I think I see the intent but doing this will also give a false sense of not introducing a breaking change when this happens. Indeed, source and binary compatibility are kept but unfortunately it is still a breaking change since behavioral compatibility is not.

I totally agree with not adding fields for everything (that's what attributes are for) but I also think that this is such a fundamental field that it should be there (like the name of the span or the localEndpoint).

I just wanted to call these out, they don't really belong to the scope of this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a "fundamental" field for one type of span, but certainly not universally applicable, which is why it shouldn't be a top level field on the Span, or the SpanData.

Copy link
Member Author

@jonatan-ivanov jonatan-ivanov Nov 16, 2022

Choose a reason for hiding this comment

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

I guess that's why it should be optional/@Nullable but to me that does not make it less important.
The fact that it is optional/not universally applicable does not justify "stringly" typing to me; if something is optional, it usually does not go into a hashmap.
I guess this is true for other parts of the OTel API/SDK, there are things that are optional but they have their own fields and they are not stringy typed, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

You're arguing with the wrong crowd here... this would be something that would need to be added in the specification, and not something we would or could unilaterally add to the java APIs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yepp, that's why I wrote that I think now it's too late for that.

Comment on lines +192 to +195
.remoteEndpoint(expectedRemoteEndpoint)
.putTag(PEER_SERVICE.getKey(), "remote-test-service")
.putTag(NET_SOCK_PEER_ADDR.getKey(), "8.8.8.8")
.putTag(NET_PEER_PORT.getKey(), "42")
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 biased though: I prefer having type-safe fields instead of pouring everything into a "hashmap".

I assume the decision to model fields in map-like structure instead of in top level fields is because it affords more ability to experiment without the consequences of adding a top level field we later wish to remove / rename / or change. This relieves bikeshedding (to some extent anyway 😄).

Additionally, since spans can potentially be used to model many many domains, the top level fields could easily get bogged down as more and more top level fields were added.

@jonatan-ivanov
Copy link
Member Author

Could somebody please re-run the GH Action, it seems it got into some trouble along the way. :)

@jack-berg jack-berg merged commit df74d4c into open-telemetry:main Nov 28, 2022
@jonatan-ivanov jonatan-ivanov deleted the zipkin-remote-endpoint branch November 28, 2022 21:08
dmarkwat pushed a commit to dmarkwat/opentelemetry-java that referenced this pull request Dec 30, 2022
* Populate Zipkin remoteEndpoint
fixes open-telemetrygh-4932

* Add conditions for creating zipkin remote endpoint

* Parameterize remote endpoint tests with span kind

* Verify INTERNAL span kind too
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.

Zipkin remoteEndpoint is missing
5 participants