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

O11Y-1528 : update sdk to use api where possible #36

Merged
merged 10 commits into from
Feb 24, 2022
Merged

O11Y-1528 : update sdk to use api where possible #36

merged 10 commits into from
Feb 24, 2022

Conversation

blakeroberts-wk
Copy link
Contributor

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

What

Any use of a class within SDK should use the API version where possible. What this means is that the type definitions of class fields, return types, and argument types should use the API version; however, instantiation of the implementation of an object must use the SDK type (as that is the type that holds the concrete implementation of the API type).

Note

I also removed the SDK class for TraceFlags in favor of a bitmap housed within the API package. I believe this is a more concise and accurate representation of the underlying mechanism of the W3C trace context and more closely aligns with the JavaScript implementation.

@Workiva/product-new-relic

@blakeroberts-wk blakeroberts-wk requested a review from a team as a code owner February 4, 2022 22:06
@rmconsole-wf
Copy link

rmconsole-wf commented Feb 4, 2022 •
edited by rmconsole6-wk
Loading

Merge Requirements Met ✅

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

General Information

Ticket(s):

Code Review(s): #36
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:
	Version: opentelemetry-dart 0.1.20
	Release Ticket(s): O11Y-1573

	This pull is considered the release of opentelemetry-dart 0.1.20 
	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 Thursday, February 24 01:51 PM CST

@aviary2-wf
Copy link

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.

keruitan-wk
keruitan-wk reviewed Feb 18, 2022
View reviewed changes
README.md
The ConsoleExporter has no requirements, and has no configuration options.
```

```dart
import 'package:opentelemetry/sdk.dart' as otel_sdk;

final exporter = otel_sdk.ConsoleExporter();
Copy link
Contributor

Choose a reason for hiding this comment

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

There's an extra "a" on line 44 under Span Processors: "Next, you will need a at least one span processor."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see that was like a cool guy pause for dramatic effect, "Next, you will need ay .. eh ... at least one span processor."

Copy link
Contributor

Choose a reason for hiding this comment

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

Also LIne 44, "A span processor is responsible for collectoring the spans you create and feeding them to the exporter." should be "collecting"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lol yes. But I am definitely adding collectoring to my vocabulary haha

@blakeroberts-wk
Copy link
Contributor Author

QA +1 tested using local build and wk-dev: https://onenr.io/0YBR6pXDqQO

@blakeroberts-wk
Copy link
Contributor Author

QA +1

changliu-wk
changliu-wk reviewed Feb 22, 2022
View reviewed changes
lib/src/api/trace/span_context.dart Outdated
import 'trace_flags.dart';
import 'trace_id.dart';
import 'trace_state.dart';
import '../../../api.dart' as api;

/// Representation of the context of the context of an individual span.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this line be "Representation of the context of an individual span"?

lib/src/api/trace/span.dart Outdated

// Retrieve the resource on this span.
Resource get resource;
api.Resource get resource;

// Retrieve the resource on this span.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment should be "Retrieve the instrumentationLibrary"?

changliu-wk
changliu-wk previously approved these changes Feb 22, 2022
View reviewed changes
@blakeroberts-wk
Copy link
Contributor Author

QA +1 here are some spans https://onenr.io/0bEjOJM08R6

@blakeroberts-wk
Copy link
Contributor Author

@Workiva/release-management-p

rmconsole-wf
rmconsole-wf approved these changes Feb 24, 2022
View reviewed changes
Copy link

@rmconsole-wf rmconsole-wf left a comment

Choose a reason for hiding this comment

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

+1 from RM

@rmconsole6-wk rmconsole6-wk merged commit 3e199a3 into master Feb 24, 2022
@rmconsole6-wk rmconsole6-wk deleted the O11Y-1528 branch February 24, 2022 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keruitan-wk keruitan-wk keruitan-wk left review comments

@changliu-wk changliu-wk changliu-wk approved these changes

@rmconsole-wf rmconsole-wf rmconsole-wf approved these changes

Assignees
No one assigned
Labels
Merge Requirements Met RM Ready
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@blakeroberts-wk @rmconsole-wf @aviary2-wf @keruitan-wk @changliu-wk @rmconsole6-wk