-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Span interface should only allow LinkedTo at creation. #349
Span interface should only allow LinkedTo at creation. #349
Conversation
e34ce34
to
f5de9d2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @VineethReddy02, thanks for taking some time and sending this PR.
Changes looks good just a couple of notes.
I wasn't following this PR and the associated issue closely, my apologies. I see there is still a spec for
|
@jmacd It is also stated at the specs that you quoted that:
And there is no specification of an I've also double-checked the java implementation and there is no method like that there. |
OK. I can see that interpretation is sensible. If @rghetia signs off, this is good to go. |
sdk/trace/trace_test.go
Outdated
{SpanContext: sc2, Attributes: []core.KeyValue{k2v2, k3v3}}, | ||
}, | ||
SpanKind: apitrace.SpanKindInternal, | ||
Links: nil, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you need to add the links with StartSpan as below
span := startSpan(tp, "Links",
apitrace.LinkedTo(sc1, key.New("key1").String("value1")),
apitrace.LinkedTo(sc1,
key.New("key2").String("value2"),
key.New("key3").String("value3"),
),
)
and then check for non-nil links
sdk/trace/trace_test.go
Outdated
DroppedLinkCount: 1, | ||
ParentSpanID: sid, | ||
Name: "LinksOverLimit/span0", | ||
Links: nil, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same thing here.
I am fine with this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM once @rghetia and @paivagustavo's test requests are made. If it would help, I'm happy to jump on Zoom to pair on the changes.
180313c
to
0fd7da6
Compare
I have remove AddLink and Link from the interface and all it refereneces and replaced AddLink with addlink, Also Removed respective unit tests Signed-off-by: vineeth <vineethpothulapati@outlook.com>
0fd7da6
to
c31b031
Compare
I made all the requested changes. But squashed all the previous commits into one commit due to some signing issue. Thanks for all the guidance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome @VineethReddy02!
Small change request but it looks good to me.
sdk/trace/trace_test.go
Outdated
apitrace.LinkedTo(sc1, key.New("key1").String("value1")) | ||
apitrace.LinkedTo(sc2, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two LinkedTo
are not being used, just remove them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, Got it. I already put them in startSpan()
function.
sdk/trace/trace_test.go
Outdated
span.Link(sc3, key.New("key3").String("value3")) | ||
apitrace.LinkedTo(sc1, key.New("key1").String("value1")) | ||
apitrace.LinkedTo(sc2, key.New("key2").String("value2")) | ||
apitrace.LinkedTo(sc3, key.New("key3").String("value3")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, just delete this.
Signed-off-by: VineethReddy02 <vineethpothulapati@outlook.com>
@iredelmeier can you please remove the block if you are okay with the latest changes? |
I have removed
AddLink
andLink
from theSpan
interface and all it references, replacedAddLink
withaddlink
, Also Removed respective unit tests forAddLink
&Link
.#329
This is my first PR to open-telemetry. Feel free to request changes or let me know if I need to catch up with some pre-requisites. :)