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.

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

feat: Rename Labels to Attributes #2523

Merged
merged 6 commits into from
Nov 8, 2021

Conversation

pirgeo
Copy link
Member

@pirgeo pirgeo commented Oct 6, 2021

Which problem is this PR solving?

  • This PR renames Attributes to Labels. No change in behavior is introduced in this PR.
  • Actually changing the type of Attributes (which are currently still string-string key-value pairs) will happen in a follow-up PR.

The opentelemetry-exporter-otlp-http package uses the Proto definitions in version 0.6.0 (https://github.com/open-telemetry/opentelemetry-proto/releases/tag/v0.6.0). This release does not have the Attributes field yet. I did not want to update the proto in this PR, since that would also mean updating the hand-rolled implementation in https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/opentelemetry-exporter-otlp-http/src/types.ts, which would make this change much more than a renaming. This is due to breaking changes released in 0.8.0 of the Proto, where the serialization of some instruments to OTLP was changed (see open-telemetry/opentelemetry-proto@v0.6.0...v0.8.0, metrics.proto, line 125 and below). Additionally, #2554 proposes using a shared proto impl, maybe even auto-generated from the proto files. Therefore, the opentelemetry-exporter-otlp-http package is in an intermediate state, where it, in some cases, uses attributes, but still exports lables, due to the old proto version.

Short description of the changes

  • s/Label/Attribute/

@pirgeo pirgeo requested a review from a team October 6, 2021 16:18
@codecov
Copy link

codecov bot commented Oct 6, 2021

Codecov Report

Merging #2523 (fd89ce9) into main (28c9e88) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2523   +/-   ##
=======================================
  Coverage   93.00%   93.00%           
=======================================
  Files         138      138           
  Lines        5092     5092           
  Branches     1095     1095           
=======================================
  Hits         4736     4736           
  Misses        356      356           
Impacted Files Coverage Δ
...exporter-prometheus/src/PrometheusLabelsBatcher.ts
...rter-prometheus/src/PrometheusAttributesBatcher.ts 100.00% <0.00%> (ø)

@vmarchaud vmarchaud marked this pull request as draft October 9, 2021 13:46
@vmarchaud
Copy link
Member

I'm converting it back to draft just to be sure its not merged before #2496

@legendecas
Copy link
Member

I'm wondering if it can be easier to review if the PR can be split into two PRs: one for renaming and one for attribute value type changes.

@legendecas
Copy link
Member

@pirgeo hi, #2496 has been merged and you may need a rebase or merging the main branch.

@pirgeo pirgeo changed the title feat: Use Attributes instead of Labels feat: Rename Attributes to Labels Oct 29, 2021
@pirgeo pirgeo marked this pull request as ready for review October 29, 2021 09:19
@pirgeo
Copy link
Member Author

pirgeo commented Oct 29, 2021

Sorry for the delay, I think it should be ready for review now!

@pirgeo pirgeo changed the title feat: Rename Attributes to Labels feat: Rename Labels to Attributes Oct 29, 2021
Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

Kind of large PR but seems pretty straightforward. Thanks for doing the work.

Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

👍

@@ -14,17 +14,17 @@
* limitations under the License.
*/

import { Labels } from './Metric';
import { Attributes } from './Metric';
Copy link
Member

Choose a reason for hiding this comment

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

According to spec, the attribute interface is now shared between metric and trace. Should we just use the Attributes from the 1.0 API?

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/common/common.md#attributes

Copy link
Member Author

Choose a reason for hiding this comment

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

That makes sense, but I believe this should happen in a follow-up PR. I assume there will be some breaking changes, especially with #2554 and open-telemetry/opentelemetry-proto#342 (including removing the deprecated labels field) coming up. I am not sure how to best navigate that change, as it seems quite big. I think what needs to happen for that to work is:

  • Update the Proto to the latest version (as the currently referenced version does not have attributes yet)
  • Either update the hand-rolled proto implementation, or set up a way of auto-generating JS code from the proto directly.
  • Then we can update the actual reference of what were labels before to the new attributes.

These things are only somewhat related to the renaming of Labels to Attributes, but without updating the underlying data structures as well as the code, I am not sure we can make this change without breaking the code or tests.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good. I'll create a follow up story for that.

@dyladan
Copy link
Member

dyladan commented Nov 3, 2021

Conflicts. Conflicts everywhere.

image

@dyladan
Copy link
Member

dyladan commented Nov 4, 2021

@open-telemetry/javascript-maintainers would like one more 👀 at this before merge. It's a simple rename so I think it's pretty safe but I might have missed something.

@@ -15,7 +15,7 @@
*/

import { SpanAttributes, HrTime } from '@opentelemetry/api';
import { Labels, ValueType } from '@opentelemetry/api-metrics';
import { Attributes as Labels, ValueType } from '@opentelemetry/api-metrics';
Copy link
Contributor

Choose a reason for hiding this comment

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

Newbie question. On line 30-39 it would not change?

Copy link
Member Author

Choose a reason for hiding this comment

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

I left it as Labels in this case, since its using the types defined for the 0.6.0 proto (until #2586 is done, probably). The field is called labels in the TS representation of the protobuf, so I kept it that way.

@pragmaticivan
Copy link
Contributor

This one is good to go?

@dyladan
Copy link
Member

dyladan commented Nov 8, 2021

Yes

@dyladan dyladan merged commit 517a31a into open-telemetry:main Nov 8, 2021
@dyladan dyladan deleted the use-attributes branch November 8, 2021 14:02
@dyladan dyladan added the enhancement New feature or request label Nov 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants