Link api - to add links to span. by lalitb · Pull Request #342 · open-telemetry/opentelemetry-cpp

lalitb · 2020-09-28T20:21:40Z

As per the specification : https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/api.md#add-links.

TBD -> Need to fix test coverage, although it's passing in forked repo PR, it seems to fail here.

codecov · 2020-09-28T20:27:19Z

Codecov Report

Merging #342 into master will decrease coverage by 0.18%.
The diff coverage is 70.58%.

@@            Coverage Diff             @@
##           master     #342      +/-   ##
==========================================
- Coverage   94.57%   94.39%   -0.19%     
==========================================
  Files         150      150              
  Lines        6768     6815      +47     
==========================================
+ Hits         6401     6433      +32     
- Misses        367      382      +15

Impacted Files	Coverage Δ
api/include/opentelemetry/trace/default_span.h	`16.66% <0.00%> (-4.77%)`	⬇️
...de/opentelemetry/ext/zpages/threadsafe_span_data.h	`96.87% <ø> (ø)`
sdk/src/trace/span.h	`100.00% <ø> (ø)`
sdk/src/trace/span.cc	`74.72% <33.33%> (-8.40%)`	⬇️
api/include/opentelemetry/trace/noop.h	`83.33% <75.00%> (-1.29%)`	⬇️
api/include/opentelemetry/trace/span.h	`100.00% <100.00%> (ø)`
api/test/trace/noop_test.cc	`100.00% <100.00%> (ø)`
sdk/include/opentelemetry/sdk/trace/recordable.h	`100.00% <100.00%> (ø)`
sdk/include/opentelemetry/sdk/trace/span_data.h	`100.00% <100.00%> (ø)`
sdk/src/trace/tracer.cc	`72.72% <100.00%> (+1.75%)`	⬆️
... and 1 more

reyang

LGTM.

pyohannes · 2020-09-28T21:43:39Z

+   *  @param attributes Link attributes
+   */
+
+  virtual void AddLink(const trace::Link &link) noexcept = 0;


Could we go without the dedicated trace::Link type altogether? We don't have a type for events, so leaving this out for links would make things more symmetrical.

Just thinking about scenarios which makes it different from Events, and having trace::Link would be helpful.

Creating a link, and then associating it with multiple spans. It would be helpful to have link as an entity in that case.

Link should be associated with the Span during it's creation, and not afterwards. As per the specification
"Links cannot be added after Span creation." With link type, it's helpful to pass sequence of links in Span constructor. Currently we do provide Span::AddLink() but it needs to be called immediately after the span creation ( probably this needs to be clearly documented).

Links cannot be added after Span creation.

I see. What do you think about completely dismissing AddLink methods, and just adding links via StartSpanOptions? This would enforce the spec requirement, I think this is a better and less ambiguous way to add links.

trace::Link as it is now is not useful. An API user couldn't even construct a trace::Link, as it's a pure interface.

@pyohannes - I agree on dismissing AddLink methods. But sending links via StartSpanOptions won't be feasible, as we need to initialize collection of links. Using std::vector/std::array may break the ABI. Better option would be to have as separate argument in StartSpan :

nostd::shared_ptr<Span> StartSpan(nostd::string_view /*name*/, const KeyValueIterable & /*attributes*/, const StartSpanOptions & /*options*/, const nostd::span<nostd::shared_ptr<Link>>& /*links*/) noexcept override

I don't have strong opinion in having dedicated Link type, but since link contains attribute pairs , and so the overloading for KeyValueIterable can be done within Link type, instead of adding more overloaded StartSpan methods. Let me know what you think ?

I like the suggestion of having this as additional argument to the StartSpan function. I'm not yet sure about the best way to realize this, but let's work towards that!

FYI this is what OpenTelemetry Python has https://github.com/open-telemetry/opentelemetry-python/blob/982b667ab7d32de45ed3b9cd6981f37a9e04975b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py#L701

@pyohannes - I have done the changes as separate PR #351 , as this was mostly a rework. Please review that PR. I am closing current PR.

pyohannes · 2020-09-29T20:38:58Z

Closes #319.

ThomsonTan · 2020-09-30T16:33:40Z

      start_steady_time{options.start_steady_time},
      has_ended_{false}
 {
+  (void)options;


Not sure why this is added back, should be removed because it was referenced in line 108 and 109.

Yeah my bad . Thanks for pointing. Have raised a separate PR #351 as this was now a rework. Please review that. Will close this one.

ThomsonTan · 2020-09-30T16:34:51Z

 }

+void Span::AddLink(const trace_api::Link &link) noexcept
+{


Declare lock_guard(mf_) to make sure it is thread-safe?

Have removed AddLink() as part of separate PR #351 . Please review that. thanks.

jlisee · 2022-05-18T21:45:55Z

Was this supposed to be closed or merged? I cannot find a way to add links to Spans otherwise.

lalitb · 2022-05-18T22:11:03Z

Was this supposed to be closed or merged? I cannot find a way to add links to Spans otherwise.

You can add link while creating the span:
https://github.com/open-telemetry/opentelemetry-cpp/blob/main/api/include/opentelemetry/trace/tracer.h#L42

Specs doesn't allow adding links once span is created.

Update dependency rules_go to v0.55.1

lalitb added 5 commits September 28, 2020 18:55

first draft

8dd7be1

fix format

375322f

fix trace_api

0513e54

fix coverage

6597099

add test

7ce23b9

lalitb requested a review from a team September 28, 2020 20:21

reyang approved these changes Sep 28, 2020

View reviewed changes

pyohannes reviewed Sep 28, 2020

View reviewed changes

lalitb added 3 commits September 29, 2020 12:55

review comment

fc5b652

more tests

92be882

add comment for span creation

dce3656

pyohannes linked an issue Sep 29, 2020 that may be closed by this pull request

Add functionality to create links on spans #319

Closed

ThomsonTan reviewed Sep 30, 2020

View reviewed changes

ThomsonTan mentioned this pull request Sep 30, 2020

REQUEST: New membership for ThomsonTan open-telemetry/community#512

Closed

6 tasks

lalitb mentioned this pull request Oct 5, 2020

Support adding links to span #351

Merged

lalitb closed this Oct 5, 2020

lalitb mentioned this pull request Nov 19, 2020

Add functionality to create links on spans #319

Closed

GerHobbelt pushed a commit to GerHobbelt/opentelemetry-cpp that referenced this pull request Aug 31, 2025

Merge pull request open-telemetry#342 from malkia/renovate/rules_go-0.x

6afcd26

Update dependency rules_go to v0.55.1

Conversation

lalitb commented Sep 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Sep 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

reyang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lalitb Sep 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lalitb Sep 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pyohannes commented Sep 29, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jlisee commented May 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lalitb commented May 18, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lalitb commented Sep 28, 2020 •

edited

Loading

codecov Bot commented Sep 28, 2020 •

edited

Loading

lalitb Sep 29, 2020 •

edited

Loading

lalitb Sep 30, 2020 •

edited

Loading

jlisee commented May 18, 2022 •

edited

Loading