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

Allow update of Span name. #23

Closed
carlosalberto opened this issue Mar 28, 2019 · 12 comments
Closed

Allow update of Span name. #23

carlosalberto opened this issue Mar 28, 2019 · 12 comments
Labels
Agreed Issues that the group agreed on.

Comments

@carlosalberto
Copy link
Contributor

In order to support a bridge to OT, we need to allow Span's name to be updated.

The method that requires this, from the OT side, is Span.setOperationName().

@bogdandrutu
Copy link
Member

bogdandrutu commented Mar 28, 2019

I think we also discussed two things:

  • How to enforce that the Span at the end() call has a name set.
  • Consider maybe updateName and enforce start span to get a name and allow updating the name later.

@carlosalberto
Copy link
Contributor Author

Yeah, was wondering about the updateName() as discussed in the call - it does feel weird, among the set*() methods though ;)

So at this moment, we are already requiring (from OT and the new API) that a Span has a name, no?When SpanBuilders are created, this is required:

// OT
Tracer.buildSpan("spanName");

// New API
Tracer.spanBuilder("spanName");

@SergeyKanzhelev
Copy link
Member

Maybe putName to align with putAttribute?

@bogdandrutu - as Carlos said, builder already requires name on construction in both. So this part should be good. Do you have anything else in mind?

@bogdandrutu
Copy link
Member

Proposal: setName vs updateName

@SergeyKanzhelev SergeyKanzhelev added the Agreed Issues that the group agreed on. label Mar 29, 2019
@bogdandrutu
Copy link
Member

Decision: Go with update, if strong opinions from community then we may revisit.

@yurishkuro
Copy link
Member

Does OC redo the sampling decision today once the span name is changed?

The Adaptive Sampling in Jaeger is done per span name (not per service), and having to support late binding of the span names was pretty painful.

@SergeyKanzhelev
Copy link
Member

@yurishkuro name update only available in C# I believe. We discussed whether we need to update sampling decision but decided against it. One of reasoning was that perhaps this kind of sampling should work on the pipe after all the data is collected. So customer will need to configure it differently when she knows that this update is probably happening.

@yurishkuro
Copy link
Member

If the service makes downstream calls, the sampling decision needs to be made before those calls. The typical use case for overriding span name is when instrumentation doesn't know the name immediately upon receiving the inbound request (e.g. if the span is created lower in the networking stack), but usually it is available and set long before the service handler is invoked and it can make downstream calls.

@SergeyKanzhelev
Copy link
Member

@yurishkuro, yes it is valid point. How did you solve it in Jaeger?

Perhaps we can only allow the change of span name via the method on tracer? So tracer will be able to re-apply sampling.

One big problem with updateName - it is typicaly that you don't know whether you will definitely need to update the name or not.

@bogdandrutu
Copy link
Member

What about having an attribute "displayName" that can be used by the backends to show better name? Or maybe a better attribute key name?

@tedsuo tedsuo added Action Required and removed Agreed Issues that the group agreed on. labels Apr 8, 2019
@tedsuo
Copy link
Contributor

tedsuo commented Apr 9, 2019

We've decided to go with Span.updateName() for this feature.

@tedsuo tedsuo added the Agreed Issues that the group agreed on. label Apr 9, 2019
@tedsuo
Copy link
Contributor

tedsuo commented Apr 9, 2019

TODO: include documentation describing the relationship between updating the operation name, and sampling decisions based on the operation name.

jkwatson pushed a commit that referenced this issue Aug 7, 2020
* Removed URLEncoder

* Fixed typo

* Added URLDecoding

* Included comment for string replacement

* Added unit tests for special characters in span names

* Resolved URL decoding issues

* Moved url decoding to parseQueryMap and updated the corresponding unit tests

* Added a README file for zPage quickstart

* Add images for README

* Updated README

* Add frontend images

* Add backend images

* Added our design doc

* Added details on package

* Reworded a few lines

* Moved DESIGN.md to a docs folder and changed gradle config to implementation

* Changed wording regarding HttpServer requirement

* Added zpages folder under docs, resolved broken image links

* Resolved comments for the design md file

* Made a few wording changes

* Wrote a benchmark test for TracezSpanBuckets (#23)

* Scaffolded logic for basic benchmark tests

* Wrote benchmark tests for TracezSpanBuckets

* Updated README with benchmark tests

* Changed the wording slightly

* Updated README file (#25)

* Wrote benchmark tests for TracezDataAggregator (#24)

* Scaffolded logic for basic benchmark tests

* Wrote benchmark tests for TracezSpanBuckets

* Updated README with benchmark tests

* Changed the wording slightly

* Added a set of benchmark tests for TracezDataAggregator

* Modified README formatting

* Changed benchmark test to negate dead code elimination

* Added Javadocs to the TracezDataAggregator benchmark tests

* Removed benchmark results from README and added a param to the TracezDataAggregator benchmark tests

* Update sdk_extensions/zpages/src/jmh/java/io/opentelemetry/sdk/extensions/zpages/TracezDataAggregatorBenchmark.java

Co-authored-by: Anuraag Agrawal <[email protected]>

* Added multiple param values for TracezDataAggregatorBenchmark

Co-authored-by: Terry Wang <[email protected]>
Co-authored-by: Anuraag Agrawal <[email protected]>
jkwatson pushed a commit that referenced this issue Aug 12, 2020
* Removed URLEncoder

* Fixed typo

* Added URLDecoding

* Included comment for string replacement

* Added unit tests for special characters in span names

* Resolved URL decoding issues

* Moved url decoding to parseQueryMap and updated the corresponding unit tests

* Added a README file for zPage quickstart

* Add images for README

* Updated README

* Add frontend images

* Add backend images

* Added our design doc

* Added details on package

* Reworded a few lines

* Moved DESIGN.md to a docs folder and changed gradle config to implementation

* Changed wording regarding HttpServer requirement

* Added zpages folder under docs, resolved broken image links

* Resolved comments for the design md file

* Made a few wording changes

* Wrote a benchmark test for TracezSpanBuckets (#23)

* Scaffolded logic for basic benchmark tests

* Wrote benchmark tests for TracezSpanBuckets

* Updated README with benchmark tests

* Changed the wording slightly

* Updated README file (#25)

* Wrote benchmark tests for TracezDataAggregator (#24)

* Scaffolded logic for basic benchmark tests

* Wrote benchmark tests for TracezSpanBuckets

* Updated README with benchmark tests

* Changed the wording slightly

* Added a set of benchmark tests for TracezDataAggregator

* Modified README formatting

* Changed benchmark test to negate dead code elimination

* Added Javadocs to the TracezDataAggregator benchmark tests

* Removed benchmark results from README and added a param to the TracezDataAggregator benchmark tests

* Update sdk_extensions/zpages/src/jmh/java/io/opentelemetry/sdk/extensions/zpages/TracezDataAggregatorBenchmark.java

Co-authored-by: Anuraag Agrawal <[email protected]>

* Added multiple param values for TracezDataAggregatorBenchmark

* Changed TraceConfigz zPage form submit to use POST request

* Added requestMethod parameter to emitHtml, limited TraceConfig change on POST request only

* Removed duplicate parse function, added test for update on POST request only

* Added separate method for processing request

* Removed unnecessary error check in tests, used try resources for inputstream

Co-authored-by: williamhu99 <[email protected]>
Co-authored-by: William Hu <[email protected]>
Co-authored-by: Anuraag Agrawal <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Agreed Issues that the group agreed on.
Projects
None yet
Development

No branches or pull requests

5 participants