-
Notifications
You must be signed in to change notification settings - Fork 894
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
Add OpenTelemetry API package layout #86
Add OpenTelemetry API package layout #86
Conversation
├── trace | ||
│ ├── propagation # headers that are the public API for trace propagation, using different encodings. | ||
│ └── samplers # is used to make decisions on `Span` sampling. | ||
├── distributedcontext |
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.
Does distributedcontext
intend to cover local context (in-process propagation) as well?
If yes, suggest to consider context
.
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.
looks like a hot topic for the Tuesday specification group call =)
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.
Based on the current Java reference implementation context (in-process) is a separate package, probably we should keep it that way as we discussed (currently we document the way java reference implementation did then later change if we agree otherwise).
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.
missed this. @mayurkale22 can you please fix?
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.
Done
@@ -0,0 +1,56 @@ | |||
# OpenTelemetry Project Package Layout |
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.
@mayurkale22 great document!
specification/package-layout.md
Outdated
### `/internal` (_Optional_) | ||
Private application and library code. | ||
|
||
### `/log` (_In the future_) |
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.
### `/log` (_In the future_) | |
### `/logs` (_In the future_) |
I think it will be plural. Like metrics and resources
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.
I think log
(singular) is a convention as package name? /cc @reyang
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.
Generally great. Few suggestions, please fix before this can be merged
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.
LGTM overall
d36823c
to
84c835b
Compare
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.
LGTM.
* Add basic package layout * Fix review comments * Add context package
…etry#86) * Explicitly forbid granting any permissions to individuals According to GDI security guidelines. * Update specification/repository.md Co-authored-by: Robert Pająk <[email protected]> Co-authored-by: Robert Pająk <[email protected]>
* Add basic package layout * Fix review comments * Add context package
Fixes #52
The best way to view this is: https://github.com/open-telemetry/opentelemetry-specification/blob/84c835b1746d14fa18d087d789728bb5e071afad/specification/package-layout.md