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

Span interface should only allow LinkedTo at creation. #349

Merged

Conversation

VineethReddy02
Copy link
Contributor

@VineethReddy02 VineethReddy02 commented Nov 24, 2019

I have removed AddLink and Link from the Span interface and all it references, replaced AddLink with addlink, Also Removed respective unit tests for AddLink & 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. :)

Copy link
Member

@paivagustavo paivagustavo left a 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.

sdk/trace/trace_test.go Show resolved Hide resolved
sdk/trace/span.go Show resolved Hide resolved
@jmacd
Copy link
Contributor

jmacd commented Nov 25, 2019

I wasn't following this PR and the associated issue closely, my apologies. I see there is still a spec for AddLink in the spec, it reads:

- An API to record a single `Link` where the `Link` properties are passed as
  arguments. This MAY be called `AddLink`.
- An API to record a single `Link` whose attributes or attribute values are
  lazily constructed, with the intention of avoiding unnecessary work if a link
  is unused. If the language supports overloads then this SHOULD be called
  `AddLink` otherwise `AddLazyLink` MAY be considered. In some languages, it might
  be easier to defer `Link` or attribute creation entirely by providing a wrapping
  class or function that returns a `Link` or formatted attributes. When providing
  a wrapping class or function it SHOULD be named `LinkFormatter`.

@paivagustavo
Copy link
Member

@jmacd It is also stated at the specs that you quoted that:

During the Span creation user MUST have the ability to record links to other Spans.

And there is no specification of an AddLink as an operation of a Span. We can compare to the Attributes that are specified in both "Span Creation" and on the "Span Operations".

I've also double-checked the java implementation and there is no method like that there.

@jmacd
Copy link
Contributor

jmacd commented Nov 25, 2019

OK. I can see that interpretation is sensible. If @rghetia signs off, this is good to go.

{SpanContext: sc2, Attributes: []core.KeyValue{k2v2, k3v3}},
},
SpanKind: apitrace.SpanKindInternal,
Links: nil,
Copy link
Contributor

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

DroppedLinkCount: 1,
ParentSpanID: sid,
Name: "LinksOverLimit/span0",
Links: nil,
Copy link
Contributor

Choose a reason for hiding this comment

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

same thing here.

@rghetia
Copy link
Contributor

rghetia commented Nov 25, 2019

OK. I can see that interpretation is sensible. If @rghetia signs off, this is good to go.

I am fine with this PR.

Copy link
Member

@iredelmeier iredelmeier left a 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.

@VineethReddy02 VineethReddy02 force-pushed the vineeth-remove-AddLink branch 2 times, most recently from 180313c to 0fd7da6 Compare November 26, 2019 02:53
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>
@VineethReddy02
Copy link
Contributor Author

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.

Copy link
Member

@paivagustavo paivagustavo left a 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.

Comment on lines 503 to 504
apitrace.LinkedTo(sc1, key.New("key1").String("value1"))
apitrace.LinkedTo(sc2,
Copy link
Member

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.

Copy link
Contributor Author

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.

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"))
Copy link
Member

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>
@rghetia
Copy link
Contributor

rghetia commented Nov 26, 2019

@iredelmeier can you please remove the block if you are okay with the latest changes?

@lizthegrey lizthegrey changed the title Remove AddLink & Link from Span Interface Span interface should only allow LinkedTo at creation. Nov 26, 2019
@lizthegrey lizthegrey merged commit a99f872 into open-telemetry:master Nov 26, 2019
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.

6 participants