-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Rename otel/label -> otel/attribute #1541
Conversation
Leave the imported name alone, to avoid a large diff and conflicts
Co-authored-by: Tyler Yahn <[email protected]>
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.
Should we use attribute
as the package name instead of attr
?
attr
is short and elegant, but there is no naming in Specification called attr
; I'm worried about this inconsistency might confuse end-users.
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 appreciate the desire to minimize the changes to review in this diff, but it seems that using import renaming to avoid changing all references from label
to attribute
results in go doc
continuing to reference label
in exported function and type signatures. I worry that this could lead to confusion when users look to import a label
package but don't find one.
Codecov Report
@@ Coverage Diff @@
## main #1541 +/- ##
=======================================
+ Coverage 77.9% 78.0% +0.1%
=======================================
Files 127 127
Lines 6582 6582
=======================================
+ Hits 5128 5136 +8
+ Misses 1209 1201 -8
Partials 245 245
|
Note that after this change, we still have some public identifiers that mention "label". Should any of these be changed too?
|
Other than the ones in the protobuf, which should probably be addressed via OTLP changes, yeah I would think those should change to |
@Aneurysm9 @XSAM can we leave those for future PRs? Some of them are related to metrics (not trace) and the change should probably cover names, comments, docs, etc. The package rename should suffice to unblock the trace release. I can file an issue to track getting rid of the concept of "labels" more broadly. |
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.
Going to merge this as-is to avoid keeping a large PR hanging out there for too long then I'll create a new issue for 1.0.0
to address the public API pieces still referencing Label
.
@@ -55,7 +55,7 @@ func (i *Iterator) Attribute() KeyValue { | |||
return i.Label() | |||
} | |||
|
|||
// IndexedLabel returns current index and label. Must be called only | |||
// IndexedLabel returns current index and attribute. Must be called only |
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.
// IndexedLabel returns current index and attribute. Must be called only | |
// IndexedAttribute returns current index and attribute. Must be called only |
Also the Label()
method should be changed to Attribute()
. Since these are public API changes I don't think we can defer them.
Fixes #1069.