Skip to content
This repository has been archived by the owner on Jul 31, 2023. It is now read-only.

allow custom for TraceID/SpanID generator #647

Closed
wants to merge 2 commits into from

Conversation

savaki
Copy link
Contributor

@savaki savaki commented Mar 28, 2018

See #643

@savaki savaki requested review from rakyll and semistrict March 28, 2018 02:22
@rakyll
Copy link
Contributor

rakyll commented Mar 28, 2018

/cc @bogdandrutu

I don't think we want to introduce a new generator as a first class citizen to this package at this point while the industry is trying to deprecate all the legacy trace headers. You should be able to generate your own IDs and use NewSpanWithRemoteParent.

@rakyll
Copy link
Contributor

rakyll commented Mar 28, 2018

I am going to close this issue. Please do not file issues here until there is consensus on the specs repo. @savaki is spending his time contributing changes that we didn't agree on.

@rakyll rakyll closed this Mar 28, 2018
@rakyll rakyll reopened this Mar 28, 2018
@rakyll
Copy link
Contributor

rakyll commented Mar 28, 2018

Closed this PR accidentally thinking I was closing the tracking issue oops. @savaki, can generating IDs and using NewSpanWithRemoteParent be good enough?

@semistrict
Copy link
Contributor

I should not have suggested this path without getting consensus, sorry for making extra work for everyone.

i had not thought of the solution using NewSpanWithRemoteParent. I think if this works (and doesn't cause XRay to have issues due to the fake parent span) then it is preferable to this for now.

@rakyll
Copy link
Contributor

rakyll commented Mar 28, 2018

@bogdandrutu pointed out that we are lacking the global TraceConfig explained at https://github.com/census-instrumentation/opencensus-specs/blob/master/trace/TraceConfig.md.

I think we can put the generator into the config which will be less invasive. See #652.

@rakyll
Copy link
Contributor

rakyll commented Mar 28, 2018

@savaki, can you revisit this PR to add a generator option to trace.Config?

@rakyll
Copy link
Contributor

rakyll commented Mar 29, 2018

I have one more concern about allowing users to set their custom ID generators. With this approach, we will make them set a global setting to change the whole behavior of the library and breaking it for all the other vendors if user want to register more than one exporter.

I think the right approach to this problem is not to touch the internal representation of IDs at OpenCensus level but to map them to a vendor-specific format when exporting and at propagation.HTTPFormat. Vendor-specific and OpenCensus IDs can be mapped back and forth.

@bogdandrutu
Copy link
Contributor

We expect that the application owner will be the one who sets both so it should not be a conflict.

The generator should generate the same sizes 16B for TravceId and 8B for SpanId

@rakyll
Copy link
Contributor

rakyll commented Mar 29, 2018

If a vendor shows up tomorrow with totally different requirements for span context, are we going to add more abstractions? I think the general solution to this problem is not to add more options but to have the mapping implemented by vendor-specific code.

I can send a PR, it shouldn't really take more than couple of lines to have the mapping done. Changing the core abstractions sounds to invasive to me given this can entirely be done in the vendor-specific package.

@bogdandrutu
Copy link
Contributor

@rakyll the mapping has to be global (distributed) so it is not that easy to implement.

@rakyll
Copy link
Contributor

rakyll commented Apr 6, 2018

Closed via #679.

@rakyll rakyll closed this Apr 6, 2018
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants