update OTLP proto to v0.9.0 #3740

codeboten · 2021-07-29T22:24:50Z

Description:
Update opentelemetry-proto to v0.9.0. As part of the change:

bumped submodule
updated sed patch file to support deprecated nullable StringKeyValue
added translation for labels to attributes in otlp_wrapper

Link to tracking Issue: Part of #3535

This is built on top of #3735

Remaining work once this PR is done:

convert labels to attributes
schema_url (adding SchemaUrl field #3759
add bytes data type (adding support for Bytes attribute type #3756)
stringify attributes Stringify non-string attributes for exporters requiring strings #3815
update FilteredLabels to FilteredAttributes translate FilteredLabels to FilteredAttributes #3822

tigrannajaryan

Looking at the changeset open-telemetry/opentelemetry-proto@v0.8.0...v0.9.0 it seems like there are a couple more things that will need to be added:

schema_url
bytes data type
Will these be added in a future PR? (I think that would be best to keep this one small).

tigrannajaryan · 2021-07-30T02:55:34Z

model/internal/otlp_wrapper.go

+func numberDataPointsLabelsToAttributes(dps []*otlpmetrics.NumberDataPoint) {
+	for i, _ := range dps {
+		if dps[i].Labels != nil {
+			dps[i].Attributes = labelsToAttributes(dps[i].Labels)


To clarify: numberDataPointsLabelsToAttributes and its callers do not attempt to merge Labels and Attributes. The assumption is that the source data is in the old format so Attributes should be empty?
Is that true and is that how MetricsCompatibilityChanges is used in the codebase? If so can we clarify this somewhere in the comments?

This has been moved out of this PR, but I think it's an important point to clarify. Based on the comments in the proto, https://github.com/open-telemetry/opentelemetry-proto/blob/48ca003a72365f5591f1acd9b065ea8316be2ade/opentelemetry/proto/metrics/v1/metrics.proto#L307-L312 what are your thoughts on the right way to move forward? I suppose we can either:

assume the Attributes are empty and override them with labels

upsert Labels into Attributes

insert Labels that are not present only

3 seems to make the most sense to me, but it's not a strong preference.

I would say we need to assume Attributes is empty:
Users cannot constructor custom protos since they are hidden. This case can happen only when we receive from the wire. So MetricsCompatibilityChanges is only called from data received on the wire which can be in new or old format but not a mix.

Depends how backwards compatible we want to be:

If only labels is set we upsert everything to the Attributes

If both are received the expectation is that they are the same, do nothing

If only attributes is received then do nothing.

On the sent side:

If we want to do the correct thing we should translate attributes to labels and send both, but this is not what we did with IntSum/IntGauge. So maybe just a CHANGELOG entry and say that we broke this?

Got it.

When receiving:

we should check if both labels and attributes are set, if so, don't do anything

if labels are set, upsert into attributes and leave labels as is.

don't do anything with the attributes only case.

When exporting, should labels always be dropped here? I worry that we would end up with inconsistent behaviour if labels are sometimes set and sometimes not.

model/pdata/metrics.go

proto_patch.sed

codeboten · 2021-07-30T17:03:54Z

Looking at the changeset open-telemetry/opentelemetry-proto@v0.8.0...v0.9.0 it seems like there are a couple more things that will need to be added:
* schema_url

* bytes data type
  Will these be added in a future PR? (I think that would be best to keep this one small).

Yes that's correct, I'm only trying to do the bare minimum to get the tests passing w/ OTLP v0.9.0, I've added the rest to a task list on this PR's description, though they should probably go in the main issue #3535, but I don't have permission to add tasks there.

bogdandrutu · 2021-08-02T18:27:52Z

Please rebase, and make more granular changes :)

codeboten · 2021-08-02T20:18:24Z

Please rebase, and make more granular changes :)

@bogdandrutu done, will add separate PRs for converting labels to attributes.

model/internal/otlp_wrapper.go

model/internal/data/protogen/metrics/v1/metrics.pb.go

…precated and nullable

Adapt code to changes in core: - config.experimental.configsource.Watchable interface open-telemetry/opentelemetry-collector#3792 - Rename CustomUnmarshable to Unmarshallable open-telemetry/opentelemetry-collector#3774 - Switched metrics from Value() to DoubleVal() open-telemetry/opentelemetry-collector#3740

* Bump smart agent version to v5.11.3 * Update CHANGELOG.md * Update core/contrib deps * Fix broken code due to renaming of CustomUnmarshable to Unmarshallable * Fix broken code due to renaming of CustomUnmarshable to Unmarshallable * Revert "Fix broken code due to renaming of CustomUnmarshable to Unmarshallable" This reverts commit de03f5e. * Revert "Fix broken code due to renaming of CustomUnmarshable to Unmarshallable" This reverts commit e09a9e3. * Update code according to changes in core Adapt code to changes in core: - config.experimental.configsource.Watchable interface open-telemetry/opentelemetry-collector#3792 - Rename CustomUnmarshable to Unmarshallable open-telemetry/opentelemetry-collector#3774 - Switched metrics from Value() to DoubleVal() open-telemetry/opentelemetry-collector#3740 Co-authored-by: Eyal Maderer <emaderer@splunk.com> Co-authored-by: Dmitry <anoshindx@gmail.com>

CoderPoet · 2022-03-17T08:05:55Z

model/internal/data/protogen/metrics/v1/metrics.pb.go

-	// The aggregation value is over the time interval (start_time_unix_nano,
-	// time_unix_nano].
+	// This field will be removed in ~3 months, on July 1, 2021.
+	Labels []v11.StringKeyValue `protobuf:"bytes,1,rep,name=labels,proto3" json:"labels"` // Deprecated: Do not use.


Has the Labels field been removed in the latest version? We still use the old version of the collector in some places. Is there any way to make it compatible?

tigrannajaryan closed this Jul 30, 2021

tigrannajaryan reopened this Jul 30, 2021

tigrannajaryan reviewed Jul 30, 2021

View reviewed changes

codeboten force-pushed the codeboten/otlp-0.9.0 branch 2 times, most recently from 9346b16 to adcd7a2 Compare July 30, 2021 21:14

codeboten mentioned this pull request Aug 2, 2021

adding support for Bytes attribute type #3756

Merged

codeboten force-pushed the codeboten/otlp-0.9.0 branch 3 times, most recently from bccda7d to 1b349bb Compare August 2, 2021 20:11

codeboten marked this pull request as ready for review August 2, 2021 20:19

codeboten requested a review from a team as a code owner August 2, 2021 20:19

codeboten requested a review from bogdandrutu August 2, 2021 20:19

bogdandrutu reviewed Aug 2, 2021

View reviewed changes

model/internal/otlp_wrapper.go Outdated Show resolved Hide resolved

tigrannajaryan reviewed Aug 3, 2021

View reviewed changes

model/internal/otlp_wrapper.go Outdated Show resolved Hide resolved

model/internal/data/protogen/metrics/v1/metrics.pb.go Outdated Show resolved Hide resolved

codeboten force-pushed the codeboten/otlp-0.9.0 branch from 2beb787 to 54b1110 Compare August 3, 2021 16:05

Alex Boten added 3 commits August 3, 2021 09:39

update proto to 0.9.0

6e1beed

make genproto, note the sed patch needed an update to support both de…

29cf4cc

…precated and nullable

add comment in sed patch

e839f7d

codeboten force-pushed the codeboten/otlp-0.9.0 branch from 54b1110 to e839f7d Compare August 3, 2021 16:39

bogdandrutu approved these changes Aug 3, 2021

View reviewed changes

bogdandrutu merged commit 9808622 into open-telemetry:main Aug 3, 2021

This was referenced Aug 9, 2021

convert incoming labels to attributes #3805

Merged

update oc internal, prometheus exporters to use attributes #3770

Merged

codeboten deleted the codeboten/otlp-0.9.0 branch August 11, 2021 23:34

CoderPoet reviewed Mar 17, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update OTLP proto to v0.9.0 #3740

update OTLP proto to v0.9.0 #3740

codeboten commented Jul 29, 2021 •

edited

tigrannajaryan left a comment

tigrannajaryan Jul 30, 2021

codeboten Aug 3, 2021

bogdandrutu Aug 9, 2021

codeboten Aug 9, 2021

codeboten commented Jul 30, 2021

bogdandrutu commented Aug 2, 2021

codeboten commented Aug 2, 2021

CoderPoet Mar 17, 2022

update OTLP proto to v0.9.0 #3740

update OTLP proto to v0.9.0 #3740

Conversation

codeboten commented Jul 29, 2021 • edited

tigrannajaryan left a comment

Choose a reason for hiding this comment

tigrannajaryan Jul 30, 2021

Choose a reason for hiding this comment

codeboten Aug 3, 2021

Choose a reason for hiding this comment

bogdandrutu Aug 9, 2021

Choose a reason for hiding this comment

codeboten Aug 9, 2021

Choose a reason for hiding this comment

codeboten commented Jul 30, 2021

bogdandrutu commented Aug 2, 2021

codeboten commented Aug 2, 2021

CoderPoet Mar 17, 2022

Choose a reason for hiding this comment

codeboten commented Jul 29, 2021 •

edited