-
Notifications
You must be signed in to change notification settings - Fork 33
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.
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
Initial branch #1
Conversation
Merge Requirements Met ✅Request Rosie to automerge this pull request by including @Workiva/release-management-p in a comment. General InformationTicket(s): None found in title Reviewers: blakeroberts-wk, tylersnavely-wf Additional InformationWatchlist Notifications: None
Note: This is a shortened report. Click here to view Rosie's full evaluation. |
Security InsightsNo security relevant content was detected by automated scans. Action Items
Questions or Comments? Reach out on Slack: #support-infosec. |
34c31bb
to
50c1f1b
Compare
@@ -0,0 +1,5 @@ | |||
import 'package:' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should delete everything vm. I think that's outside the scope of this project and I don't want to support it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For sure. I was planning to have us only keep the opentelemetry_api
and opentelemetry_sdk
sections, and move the opentelemetry_context
into them. I think we can clear this up in a follow up PR. My intentions were to just include the original work so we could have a starting place for discussions and a place where we can start working async.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(sorry, late to the party 😄) totally agree on focusing on the browser use case for now, but it may be worth keeping in mind along the way whether there's anything in the implementations that requires browser APIs. If we can instead write it in a platform-agnostic way, then it becomes usable from VM, web, and flutter, which would be nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
absolutely. I'd hope we wouldn't have any code that depends on browser specific APIs. If that becomes the case, it should be easy enough to label the overarching interface implementation as browser specific and leave room for a vm implementation
QA +1 |
@Workiva/release-management-p |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 from RM
@evanweible-wf had started this effort many months ago, and we want to pick it up and push it forward with dedicated resources.
Looking at where he left off, the code is in a pretty good state; much better than starting from scratch. The work is based on the OpenTelemetry Specification for language implementation.
This PR just copies all of Evan's work as a base for us to work off of. The first commit was authored by him, and then I added some new repo boilerplate stuff in the second.