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.

Sign up for GitHub

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

Jump to bottom

Add Span.add_link() method to add link after span start. #3618

Merged
merged 14 commits into from
Feb 29, 2024
Merged

Add Span.add_link() method to add link after span start. #3618

merged 14 commits into from
Feb 29, 2024

Conversation

chittalpatel
Copy link
Contributor

@chittalpatel chittalpatel commented Jan 5, 2024 •
edited
Loading

Description

Implementation of adding links after span start, defined in: open-telemetry/opentelemetry-specification#3678

Fixes #3607

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Added two new unit tests in opentelemetry-sdk.

  • TestSpan::test_add_link
  • TestSpan::test_add_link_with_invalid_span_context

Does This PR Require a Contrib Repo Change?

  • No.

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@chittalpatel chittalpatel requested a review from a team January 5, 2024 23:36
pmcollins
pmcollins approved these changes Jan 9, 2024
View reviewed changes
Copy link
Member

@pmcollins pmcollins left a comment

Choose a reason for hiding this comment

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

Checked code for style, correctness, and conformance to spec: LGTM but would like someone else to look as well.

@pyohannes pyohannes mentioned this pull request Jan 29, 2024
Open
@ocelotl
Copy link
Contributor

ocelotl commented Jan 31, 2024 •
edited
Loading

@chittalpatel @srikanthccv please mark the conversations as resolved if they are ✌️

aabmass
aabmass requested changes Feb 1, 2024
View reviewed changes
Copy link
Member

@aabmass aabmass left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, just one comment

opentelemetry-api/src/opentelemetry/trace/span.py Outdated Show resolved Hide resolved
@pyohannes pyohannes mentioned this pull request Feb 7, 2024
Closed
@ocelotl ocelotl self-assigned this Feb 14, 2024
@ocelotl ocelotl force-pushed the 3607-add-links-to-span-after-creation branch from 1921dbe to 4f52379 Compare February 14, 2024 18:17
@ocelotl
Copy link
Contributor

ocelotl commented Feb 14, 2024

@aabmass PTAL, are you ok with this PR as it is right now or do you want further changes?

@vringar vringar mentioned this pull request Feb 21, 2024
Draft
aabmass
aabmass approved these changes Feb 21, 2024
View reviewed changes
Copy link
Member

@aabmass aabmass left a comment

Choose a reason for hiding this comment

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

Thanks LGTM!

@ocelotl ocelotl enabled auto-merge (squash) February 29, 2024 02:58
auto-merge was automatically disabled February 29, 2024 03:04

Pull Request is not mergeable

@ocelotl ocelotl merged commit 9481893 into open-telemetry:main Feb 29, 2024
232 checks passed
@carlosalberto carlosalberto mentioned this pull request Mar 7, 2024
Merged
@srikanthccv srikanthccv mentioned this pull request Mar 20, 2024
Closed
@emdneto emdneto mentioned this pull request Jun 30, 2024
Merged
@kevinji kevinji mentioned this pull request Aug 13, 2024
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ocelotl ocelotl ocelotl approved these changes

@aabmass aabmass aabmass approved these changes

@pmcollins pmcollins pmcollins approved these changes

@srikanthccv srikanthccv Awaiting requested review from srikanthccv

Assignees

@ocelotl ocelotl

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add links to span after creation via Span.add_link() method
5 participants
@chittalpatel @ocelotl @pmcollins @aabmass @srikanthccv