O11Y-1665: Stub out remaining interface methods in opentelemetry-dart #43

michaelyeager-wf · 2022-04-04T22:15:28Z

Notes

A review of the classes specified in the ticket revealed the following missing API:

SpanExporter did not include a forceFlush method.
Span:
- Did not include API to specify or retrieve a SpanKind.
- Did not include API to add an Event to a Span.
- Did not include API to specify a link when creating a Span.
Tracer:
- Did not include API to specify a SpanKind when creating a Span.
- Did not include API to specify a start time when creating a Span.
- Did not include API to specify a link when creating a Span.
Sampler:
- Did not include API to specify a SpanKind when deciding whether to sample.
- Did not include API to specify links when deciding whether to sample.

This PR stubs out or otherwise makes available the API outlined above. Other work will be required to make use of this API and to fully implement its features.

Reviewers

@Workiva/product-new-relic

rmconsole-wf · 2022-04-04T22:15:59Z

Merge Requirements Met ✅

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

General Information

Ticket(s):

O11Y-1665 | remove

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

Reviewers: changliu-wk, michaelyeager-wf, keruitan-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.4.2
	Version after merge: opentelemetry-dart 0.5.0
	Release Ticket(s): O11Y-1717

	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 Friday, April 15 02:01 PM CST

aviary3-wk · 2022-04-04T22:16:52Z

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-04-04T22:17:24Z

Public API Changes

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

@@ line 3: package:opentelemetry/src/api/exporters/span_exporter.dart @@
abstract class SpanExporter
+     void forceFlush()
//    Adding abstract members breaks all subclasses.

@@ line 4: package:opentelemetry/src/sdk/trace/samplers/always_on_sampler.dart @@
class AlwaysOnSampler implements Sampler
-     SamplingResult shouldSample(Context context, TraceId traceId, String spanName, bool spanIsRemote, List<Attribute> spanAttributes)
+     SamplingResult shouldSample(Context context, TraceId traceId, String spanName, SpanKind spanKind, bool spanIsRemote, List<Attribute> spanAttributes, List<SpanLink> spanLinks)
//    `type` of `spanIsRemote` has changed.
//    Changing a parameter signature is a major change.

@@ line 11: package:opentelemetry/src/api/trace/span.dart @@
abstract class Span
-     String get name
+     String name
//    Added setter for `name`.
//    Adding a field is a minor change.

+     void addEvent(String name, Int64 timestamp, {List<Attribute> attributes})
//    Adding abstract members breaks all subclasses.

+     SpanKind get kind
//    Adding abstract members breaks all subclasses.

@@ line 2: package:opentelemetry/src/api/trace/trace_state.dart @@
abstract class TraceState
+     void remove(String key)
//    Adding abstract members breaks all subclasses.

@@ line 5: package:opentelemetry/src/api/trace/sampler.dart @@
abstract class Sampler
-     SamplingResult shouldSample(Context context, TraceId traceId, String spanName, bool spanIsRemote, List<Attribute> spanAttributes)
+     SamplingResult shouldSample(Context context, TraceId traceId, String spanName, SpanKind spanKind, bool spanIsRemote, List<Attribute> spanAttributes, List<SpanLink> spanLinks)
//    `type` of `spanIsRemote` has changed.
//    Changing a parameter signature is a major change.

Click to see 11 more API Changes

@@ line 4: package:opentelemetry/src/sdk/trace/samplers/parent_based_sampler.dart @@
class ParentBasedSampler implements Sampler
-     SamplingResult shouldSample(Context context, TraceId traceId, String spanName, bool spanIsRemote, List<Attribute> spanAttributes)
+     SamplingResult shouldSample(Context context, TraceId traceId, String spanName, SpanKind spanKind, bool spanIsRemote, List<Attribute> spanAttributes, List<SpanLink> spanLinks)
//    `type` of `spanIsRemote` has changed.
//    Changing a parameter signature is a major change.

@@ line 4: package:opentelemetry/src/sdk/trace/samplers/always_off_sampler.dart @@
class AlwaysOffSampler implements Sampler
-     SamplingResult shouldSample(Context context, TraceId traceId, String spanName, bool spanIsRemote, List<Attribute> spanAttributes)
+     SamplingResult shouldSample(Context context, TraceId traceId, String spanName, SpanKind spanKind, bool spanIsRemote, List<Attribute> spanAttributes, List<SpanLink> links)
//    `type` of `spanIsRemote` has changed.
//    Changing a parameter signature is a major change.

@@ line 7: package:opentelemetry/src/api/trace/tracer.dart @@
abstract class Tracer
-     Span startSpan(String name, {Context context, List<Attribute> attributes})
+     Span startSpan(String name, {Context context, SpanKind kind, List<Attribute> attributes, List<SpanLink> links, Int64 startTime})
//    `startSpan` is abstract.
//    Changing the signature of an abstract member breaks all subclasses.

@@ line 5: package:opentelemetry/src/api/trace/span.dart @@
+  enum SpanKind { server, client, producer, consumer, internal }
// Adding an enum is a minor change.

@@ line 12: package:opentelemetry/src/api/trace/nonrecording_span.dart @@
class NonRecordingSpan implements Span
-     String get name
+     String name
//    Added setter for `name`.
//    Adding a field is a minor change.

+     void addEvent(String name, Int64 timestamp, {List<Attribute> attributes})
//    Adding a method is a minor change.

+     SpanKind get kind
//    Adding a field is a minor change.

@@ line 11: package:opentelemetry/src/sdk/trace/exporters/collector_exporter.dart @@
class CollectorExporter implements SpanExporter
+     void forceFlush()
//    Adding a method is a minor change.

@@ line 6: package:opentelemetry/src/sdk/trace/trace_state.dart @@
class TraceState implements TraceState
+     void remove(String key)
//    Adding a method is a minor change.

@@ line 7: package:opentelemetry/src/sdk/trace/span.dart @@
class Span implements Span
-     Span Span(String name, SpanContext _spanContext, SpanId _parentSpanId, List<SpanProcessor> _processors, Resource _resource, InstrumentationLibrary _instrumentationLibrary, {List<Attribute> attributes, SpanLimits spanlimits})
+     Span Span(String name, SpanContext _spanContext, SpanId _parentSpanId, List<SpanProcessor> _processors, Resource _resource, InstrumentationLibrary _instrumentationLibrary, {SpanKind kind, List<Attribute> attributes, List<SpanLink> links, SpanLimits spanlimits, Int64 startTime})
//    `kind` was added.
//    Adding an optional parameter is a minor change.

+     void addEvent(String name, Int64 timestamp, {List<Attribute> attributes})
//    Adding a method is a minor change.

+     SpanKind get kind
//    Adding a field is a minor change.

@@ line 5: package:opentelemetry/src/sdk/trace/exporters/console_exporter.dart @@
class ConsoleExporter implements SpanExporter
+     void forceFlush()
//    Adding a method is a minor change.

@@ line 5: package:opentelemetry/src/sdk/trace/tracer.dart @@
class Tracer implements Tracer
-     Span startSpan(String name, {Context context, List<Attribute> attributes})
+     Span startSpan(String name, {Context context, SpanKind kind, List<Attribute> attributes, List<SpanLink> links, Int64 startTime})
//    `kind` was added.
//    Adding an optional parameter is a minor change.

@@ line 3: package:opentelemetry/src/api/trace/span_link.dart @@
+  class SpanLink
// Adding a class is a minor change.

Showing results for 53d3e2f

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 Apr 14 at 20:47:32

blakeroberts-wk · 2022-04-05T16:25:35Z

lib/src/api/trace/sampler.dart

      bool spanIsRemote, // ignore: avoid_positional_boolean_parameters
-      api.Attributes spanAttributes);
+      api.Attributes spanAttributes,
+      List<api.SpanLink> spanLinks);


should we make the args to this named parameters?

I don't think so. The spec calls out that these parameters are required: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/sdk.md#shouldsample

We also have all of the spec-"required" parameters in the signature now. If we're looking to avoid breaking API, I'd be in favor of using named parameters for additional parameters.

these parameters are required

Well, excepting spanIsRemote, which isn't in the spec at all. I suspect that SpanKind is intended to fulfill that role. Should we then make spanIsRemote a named parameter for the possibility that it's removed once we do the work to actually start using SpanKind for things? It's hard to say.

Perhaps remove spanIsRemote?

lib/src/api/trace/nonrecording_span.dart

lib/src/sdk/trace/span.dart

changliu-wk · 2022-04-13T18:57:47Z

Should have a API in Span named updateName()
TraceState.dart should have API to delete a key/value pair.

michaelyeager-wf · 2022-04-13T19:06:58Z

Should have a API in Span named updateName()

The name property has an exposed setter, so this can be accomplished by mySpan.name = "New Name";.

TraceState.dart should have API to delete a key/value pair.

👍

lib/src/api/trace/span_link.dart

changliu-wk · 2022-04-13T19:35:06Z

The name property has an exposed setter, so this can be accomplished by mySpan.name = "New Name";.

api.Span can't do it.

- Implement kind getters for Span classes.

- Add API to delete items from TraceState.

- Modified types in API for SpanLink.

- Exposing API to set Span name.

- Correcting merge conflicts with the master branch. - Type corrections in sampler unit tests.

changliu-wk · 2022-04-14T13:06:45Z

NonRecordingSpan should not be overridable.

michaelyeager-wf · 2022-04-14T19:29:04Z

The API MUST provide an operation for wrapping a SpanContext with an object implementing the Span interface.

This is done by creating a NonRecordingSpan using the SpanContext in question. For example, https://github.com/Workiva/opentelemetry-dart/blob/master/lib/src/sdk/trace/propagation/w3c_trace_context_propagator.dart#L56

NonRecordingSpan should not be overridable.

Our usages of this class are limited to:

Wrapping incoming context, as with the propagator in the example above.
Created when starting a span with a NoopTracer.

Neither of these operations can yield anything other than a NonRecordingSpan. Could you be more specific as to where use of NonRecordingSpan can be overridden by a consumer? Is there a place we have missed?

lib/src/api/trace/span.dart

lib/src/sdk/trace/span.dart

lib/src/api/trace/trace_state.dart

- Correcting types in API for adding events.

michaelyeager-wf · 2022-04-15T18:58:44Z

Confirmed traces are properly created and forwarded to New Relic.

QA +1

michaelyeager-wf · 2022-04-15T19:01:32Z

@Workiva/release-management-p

rmconsole-wf

+1 from RM

michaelyeager-wf requested a review from a team as a code owner April 4, 2022 22:15

rmconsole-wf added the Merge Requirements Unmet label Apr 4, 2022

semveraudit-wf added the semver: Major label Apr 4, 2022

blakeroberts-wk reviewed Apr 5, 2022

View reviewed changes

lib/src/api/trace/nonrecording_span.dart Outdated Show resolved Hide resolved

blakeroberts-wk reviewed Apr 5, 2022

View reviewed changes

lib/src/sdk/trace/span.dart Outdated Show resolved Hide resolved

michaelyeager-wf force-pushed the myeager-wf/O11Y-1665 branch from 615027e to aa5e2bb Compare April 13, 2022 15:43

changliu-wk reviewed Apr 13, 2022

View reviewed changes

lib/src/api/trace/span_link.dart Outdated Show resolved Hide resolved

Michael Yeager added 6 commits April 13, 2022 16:45

O11Y-1665: Stub out remaining interface methods in opentelemetry-dart

4678444

O11Y-1665: Stub out remaining interface methods in opentelemetry-dart

f63c796

- Implement kind getters for Span classes.

O11Y-1665: Stub out remaining interface methods in opentelemetry-dart

f23e039

- Add API to delete items from TraceState.

O11Y-1665: Stub out remaining interface methods in opentelemetry-dart

5d2fb7b

- Modified types in API for SpanLink.

O11Y-1665: Stub out remaining interface methods in opentelemetry-dart

1eb865c

- Exposing API to set Span name.

O11Y-1665: Stub out remaining interface methods in opentelemetry-dart

eb7182f

- Correcting merge conflicts with the master branch. - Type corrections in sampler unit tests.

michaelyeager-wf force-pushed the myeager-wf/O11Y-1665 branch from 80f223e to eb7182f Compare April 13, 2022 22:03

changliu-wk previously approved these changes Apr 14, 2022

View reviewed changes

michaelyeager-wf commented Apr 14, 2022

View reviewed changes

lib/src/api/trace/span.dart Outdated Show resolved Hide resolved

michaelyeager-wf commented Apr 14, 2022

View reviewed changes

lib/src/sdk/trace/span.dart Outdated Show resolved Hide resolved

keruitan-wk reviewed Apr 14, 2022

View reviewed changes

lib/src/api/trace/trace_state.dart Outdated Show resolved Hide resolved

O11Y-1665: Stub out remaining interface methods in opentelemetry-dart

53d3e2f

- Correcting types in API for adding events.

michaelyeager-wf dismissed changliu-wk’s stale review via 53d3e2f April 14, 2022 20:44

michaelyeager-wf removed the semver: Major label Apr 14, 2022

michaelyeager-wf added the semver: Minor label Apr 14, 2022

semveraudit-wf added semver: Major and removed semver: Minor labels Apr 14, 2022

michaelyeager-wf added semver: Minor and removed semver: Major labels Apr 14, 2022

changliu-wk approved these changes Apr 14, 2022

View reviewed changes

rmconsole-wf added Merge Requirements Met Merge Requirements Unmet and removed Merge Requirements Unmet Merge Requirements Met labels Apr 14, 2022

keruitan-wk approved these changes Apr 15, 2022

View reviewed changes

blakeroberts-wk approved these changes Apr 15, 2022

View reviewed changes

rmconsole-wf added Merge Requirements Met and removed Merge Requirements Unmet labels Apr 15, 2022

rmconsole-wf added the RM Ready label Apr 15, 2022

rmconsole-wf approved these changes Apr 15, 2022

View reviewed changes

rmconsole2-wf merged commit b72a4b6 into Workiva:master Apr 15, 2022

rmconsole2-wf deleted the myeager-wf/O11Y-1665 branch April 15, 2022 19:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

O11Y-1665: Stub out remaining interface methods in opentelemetry-dart #43

O11Y-1665: Stub out remaining interface methods in opentelemetry-dart #43

michaelyeager-wf commented Apr 4, 2022

rmconsole-wf commented Apr 4, 2022 •

edited by rmconsole2-wf

Loading

aviary3-wk commented Apr 4, 2022

semveraudit-wf commented Apr 4, 2022 •

edited

Loading

blakeroberts-wk Apr 5, 2022

michaelyeager-wf Apr 5, 2022

michaelyeager-wf Apr 5, 2022

michaelyeager-wf Apr 5, 2022

changliu-wk Apr 13, 2022

changliu-wk commented Apr 13, 2022 •

edited

Loading

michaelyeager-wf commented Apr 13, 2022

changliu-wk commented Apr 13, 2022 •

edited

Loading

changliu-wk commented Apr 14, 2022

michaelyeager-wf commented Apr 14, 2022

michaelyeager-wf commented Apr 15, 2022

michaelyeager-wf commented Apr 15, 2022

rmconsole-wf left a comment

O11Y-1665: Stub out remaining interface methods in opentelemetry-dart #43

O11Y-1665: Stub out remaining interface methods in opentelemetry-dart #43

Conversation

michaelyeager-wf commented Apr 4, 2022

Notes

Reviewers

rmconsole-wf commented Apr 4, 2022 • edited by rmconsole2-wf Loading

Merge Requirements Met ✅

General Information

Additional Information

aviary3-wk commented Apr 4, 2022

Security Insights

Action Items

semveraudit-wf commented Apr 4, 2022 • edited Loading

Public API Changes

blakeroberts-wk Apr 5, 2022

Choose a reason for hiding this comment

michaelyeager-wf Apr 5, 2022

Choose a reason for hiding this comment

michaelyeager-wf Apr 5, 2022

Choose a reason for hiding this comment

michaelyeager-wf Apr 5, 2022

Choose a reason for hiding this comment

changliu-wk Apr 13, 2022

Choose a reason for hiding this comment

changliu-wk commented Apr 13, 2022 • edited Loading

michaelyeager-wf commented Apr 13, 2022

changliu-wk commented Apr 13, 2022 • edited Loading

changliu-wk commented Apr 14, 2022

michaelyeager-wf commented Apr 14, 2022

michaelyeager-wf commented Apr 15, 2022

michaelyeager-wf commented Apr 15, 2022

rmconsole-wf left a comment

Choose a reason for hiding this comment

rmconsole-wf commented Apr 4, 2022 •

edited by rmconsole2-wf

Loading

semveraudit-wf commented Apr 4, 2022 •

edited

Loading

changliu-wk commented Apr 13, 2022 •

edited

Loading

changliu-wk commented Apr 13, 2022 •

edited

Loading