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

Add get data of span method #1008

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add get data of span method #1008

wants to merge 2 commits into from

Conversation

yore-new
Copy link

@yore-new yore-new commented Jan 3, 2019

return the data of span to allow accessing all info of a span.
just return the value of data instead a pointer to avoid changing by user.
in order to solve #1007 .

return the data of span to allow accessing all info of a span.

just return the value of data instead a pointer to avoid changing by user.
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@yore-new
Copy link
Author

yore-new commented Jan 3, 2019

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@basvanbeek
Copy link
Member

As explained in #1007 there is no need to provide access to the entire span data payload for correlation purposes. This is what SpanContext is for. The other problem is you return a shallow copy of span data which would allow for mutating for instance the Attributes map (and get some concurrent access nasties).
For now, I do not see a reason for allowing access to even a full copy of span data.

span data will not init if span is not sampled and not enable
local span storage. so add a flag to force generate span data.
this operation will have a little more memery overhead, but
it can be use to get all information when we get a span. we can
use the information to do logging and rebuild the trace from
logs.

also change SpanData method to use makeSpanData result, because
shallow copy of span.data would allow for mutating for instance
the Attributes map (and get some concurrent access nasties).
@rakyll
Copy link
Contributor

rakyll commented Jan 29, 2019

Why do we need to give SetForceGenerateSpanData as an option to the user?

@bogdandrutu, do we have any requirements about immutability of the span data?

@sydnash
Copy link

sydnash commented Nov 12, 2019

As explained in #1007 there is no need to provide access to the entire span data payload for correlation purposes. This is what SpanContext is for. The other problem is you return a shallow copy of span data which would allow for mutating for instance the Attributes map (and get some concurrent access nasties).
For now, I do not see a reason for allowing access to even a full copy of span data.

but i cann't get parent span id from SpanContext, could add a parent span id to the SpanContext?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants