O11Y-1750: implement spanLinks #56

changliu-wk · 2022-05-09T14:10:03Z

https://jira.atl.workiva.net/browse/O11Y-1750

This PR implements spanLinks.

Constructor calls _addLinks to implement spanLinks in a span.
Serialize span links by using Protocol Buffers when exporting spans.

There are three limits with spanLinks.

Span can't have more links than maxNumLink.
Span can't have more attributes in a link than maxNumAttributesPerLink.
The string length of each spanLink attribute can't be longer than maxNumAttributeLength.

@Workiva/observability

rmconsole-wf · 2022-05-09T14:10:25Z

Merge Requirements Met ✅

Request Rosie to automerge this pull request by including @Workiva/release-management-p in a comment.

General Information

Ticket(s):

O11Y-1750 | remove

Code Review(s): #56
Release Image Tags:

Reviewers: changliu-wk, blakeroberts-wk

Additional Information

Watchlist Notifications: None
Pull Requests included in release:

	When this pull is merged I will add it to the following release:
	Current version: opentelemetry-dart 0.10.1
	Version after merge: opentelemetry-dart 0.11.0
	Release Ticket(s): O11Y-1817

	This pull is considered a release pull
	The options defined for this repo will be carried out

Note: This is a shortened report. Click here to view Rosie's full evaluation.
Last updated on Wednesday, May 25 02:01 PM CST

aviary2-wf · 2022-05-09T14:12:11Z

Security Insights

No security relevant content was detected by automated scans.

Action Items

Review PR for security impact; comment "security review required" if needed or unsure
Verify aviary.yaml coverage of security relevant code

Questions or Comments? Reach out on Slack: #support-infosec.

semveraudit-wf · 2022-05-10T19:37:20Z

Public API Changes

Recommendation: ‼️ Major version bump (fyi @Workiva/semver-audit-group )

@@ line 8: package:opentelemetry/src/sdk/trace/span.dart @@
class Span implements Span
-     Span Span(String name, SpanContext _spanContext, SpanId _parentSpanId, List<SpanProcessor> _processors, TimeProvider _timeProvider, Resource _resource, InstrumentationLibrary _instrumentationLibrary, {SpanKind kind, List<Attribute> attributes, List<SpanLink> links, Context parentContext, SpanLimits spanlimits, Int64 startTime})
+     Span Span(String name, SpanContext _spanContext, SpanId _parentSpanId, List<SpanProcessor> _processors, TimeProvider _timeProvider, Resource _resource, InstrumentationLibrary _instrumentationLibrary, {SpanKind kind, List<Attribute> attributes, List<SpanLink> links, Context parentContext, SpanLimits limits, Int64 startTime})
//    `spanlimits` was removed.
//    Removing a parameter is a major change.

+     List<SpanLink> get links
//    Adding a field is a minor change.

Showing results for 23550bc

Powered by semver-audit-service. Please report any problems by filing an issue.
Reported by the dart semver audit client 2.2.2
Browse public API.

Last edited UTC May 25 at 16:19:33

changliu-wk · 2022-05-10T20:53:21Z

QA +1 Unit test cases passed, used Test App tested new update, didn't break anything. Span links seems no showing in NewRelic.

blakeroberts-wk · 2022-05-10T21:18:09Z

Span links seems no showing in NewRelic.

New Relic has mentioned in the past that they do not support span links. I assume they just drop that data.

blakeroberts-wk · 2022-05-10T21:23:25Z

lib/src/sdk/trace/span.dart

-      sdk.SpanLimits spanlimits,
+      sdk.SpanLimits limits,


Because we expose the type sdk.Span, this is a breaking change. I think it makes sense to either make this constructor not visible from our public API, or use one of the annotations here:

https://pub.dev/documentation/meta/latest/meta/protected-constant.html

https://pub.dev/documentation/meta/latest/meta/internal-constant.html

I just wanted this parameter have same naming convention with others, it won't break wo11y-dart, which just uses TracerProvider, and didn't think we exposed the type sdk.Span. But I think perhaps no customer use this type so far. Of course, if you think this is not a good change, we can keep it as it was.

If we hide this constructor, is it also a breaking change?

I will try to hide it as you suggested to see how it works.

I don't have any issue with the name change. But going forward we should hide the sdk.Span constructor from consumers (however we do that) so we can continue to make changes to the constructor's arguments post 1.0 release if needed.

One option we may be able to explore is making the constructor private then supplying a protected/internal static method that invokes the private constructor. This static method would be what a tracer invokes to construct a span.

One option we may be able to explore is making the constructor private then supplying a protected/internal static method that invokes the private constructor. This static method would be what a tracer invokes to construct a span.

I think this is a right way to do it. I will try it.

I tried to let constructor to be a private method. But since it is an implementation of the abstract class, it would be safe to be remove from exported sdk API.

Because this PR in wo11y-dart, we need to wait until O11Y-1786 is done to remove sdk span from exporting. I created a ticket for this work O11Y-1825.

rmconsole-wf · 2022-05-11T19:04:14Z

@changliu-wk This pull request has merge conflicts, please resolve.

rmconsole-wf · 2022-05-12T17:09:10Z

@changliu-wk This pull request has merge conflicts, please resolve.

blakeroberts-wk · 2022-05-25T15:12:46Z

lib/src/sdk/trace/span.dart

@@ -168,6 +164,58 @@ class Span implements api.Span {
    throw UnimplementedError();
  }

+  // This method just can be called once during construction.
+  static List<api.SpanLink> _addLinks(


nit: perhaps a method name like _applyLinkLimits is a more descriptive method name?

changliu-wk · 2022-05-25T16:36:26Z

QA +1

changliu-wk · 2022-05-25T19:01:01Z

@Workiva/release-management-p

rmconsole-wf

+1 from RM

changliu-wk requested a review from a team as a code owner May 9, 2022 14:10

rmconsole-wf added the Merge Requirements Unmet label May 9, 2022

changliu-wk force-pushed the Chang/O11Y-1750 branch from 3a636f3 to 23e5f41 Compare May 10, 2022 13:54

implement span links

be14434

changliu-wk force-pushed the Chang/O11Y-1750 branch from 23e5f41 to be14434 Compare May 10, 2022 19:33

semveraudit-wf added the semver: Major label May 10, 2022

blakeroberts-wk reviewed May 10, 2022

View reviewed changes

rmconsole-wf added the Merge Conflicts label May 11, 2022

rmconsole-wf added Skynet 2.0 Failed and removed Merge Conflicts Skynet 2.0 Failed labels May 11, 2022

changliu-wk force-pushed the Chang/O11Y-1750 branch from 3934866 to eb71c1f Compare May 12, 2022 17:02

merge master

4473d90

changliu-wk force-pushed the Chang/O11Y-1750 branch from eb71c1f to 4473d90 Compare May 12, 2022 17:09

rmconsole-wf added the Merge Conflicts label May 12, 2022

Merge branch 'master' into Chang/O11Y-1750

f3aea3f

rmconsole-wf removed the Merge Conflicts label May 12, 2022

blakeroberts-wk previously approved these changes May 25, 2022

View reviewed changes

change method name

23550bc

changliu-wk dismissed blakeroberts-wk’s stale review via 23550bc May 25, 2022 16:16

changliu-wk added semver: Minor and removed semver: Major labels May 25, 2022

blakeroberts-wk approved these changes May 25, 2022

View reviewed changes

rmconsole-wf added Merge Requirements Met and removed Merge Requirements Unmet labels May 25, 2022

rmconsole-wf added the RM Ready label May 25, 2022

rmconsole-wf approved these changes May 25, 2022

View reviewed changes

rmconsole2-wf merged commit b141974 into master May 25, 2022

rmconsole2-wf deleted the Chang/O11Y-1750 branch May 25, 2022 19:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

O11Y-1750: implement spanLinks #56

O11Y-1750: implement spanLinks #56

changliu-wk commented May 9, 2022 •

edited

Loading

rmconsole-wf commented May 9, 2022 •

edited by rmconsole2-wf

Loading

aviary2-wf commented May 9, 2022

semveraudit-wf commented May 10, 2022 •

edited

Loading

changliu-wk commented May 10, 2022

blakeroberts-wk commented May 10, 2022

blakeroberts-wk May 10, 2022

changliu-wk May 11, 2022

blakeroberts-wk May 11, 2022

changliu-wk May 11, 2022 •

edited

Loading

changliu-wk May 11, 2022

changliu-wk May 25, 2022

rmconsole-wf commented May 11, 2022

rmconsole-wf commented May 12, 2022

blakeroberts-wk May 25, 2022

changliu-wk commented May 25, 2022

changliu-wk commented May 25, 2022

rmconsole-wf left a comment

O11Y-1750: implement spanLinks #56

O11Y-1750: implement spanLinks #56

Conversation

changliu-wk commented May 9, 2022 • edited Loading

rmconsole-wf commented May 9, 2022 • edited by rmconsole2-wf Loading

Merge Requirements Met ✅

General Information

Additional Information

aviary2-wf commented May 9, 2022

Security Insights

Action Items

semveraudit-wf commented May 10, 2022 • edited Loading

Public API Changes

changliu-wk commented May 10, 2022

blakeroberts-wk commented May 10, 2022

blakeroberts-wk May 10, 2022

Choose a reason for hiding this comment

changliu-wk May 11, 2022

Choose a reason for hiding this comment

blakeroberts-wk May 11, 2022

Choose a reason for hiding this comment

changliu-wk May 11, 2022 • edited Loading

Choose a reason for hiding this comment

changliu-wk May 11, 2022

Choose a reason for hiding this comment

changliu-wk May 25, 2022

Choose a reason for hiding this comment

rmconsole-wf commented May 11, 2022

rmconsole-wf commented May 12, 2022

blakeroberts-wk May 25, 2022

Choose a reason for hiding this comment

changliu-wk commented May 25, 2022

changliu-wk commented May 25, 2022

rmconsole-wf left a comment

Choose a reason for hiding this comment

changliu-wk commented May 9, 2022 •

edited

Loading

rmconsole-wf commented May 9, 2022 •

edited by rmconsole2-wf

Loading

semveraudit-wf commented May 10, 2022 •

edited

Loading

changliu-wk May 11, 2022 •

edited

Loading