Update flags API #5602

TylerHelmuth · 2022-06-27T16:32:50Z

Description:
This PR updates the Flags API to hide all references to the Flags themselves. Instead, setting flags and checking if a flag is set is handled via functions that know about the flag being set/checked.

Link to tracking Issue:
Fixes #5443

Testing:
Updated unit tests

TylerHelmuth · 2022-06-27T16:34:21Z

@bogdandrutu @dmitryax is this what you had in mind with #5443? This implementation is currently a breaking change to show final state, but I think we'd need to handle deprecating HasFlag and the original constructor.

codecov · 2022-06-27T16:36:26Z

Codecov Report

Merging #5602 (1fca8d8) into main (b2dc376) will decrease coverage by 0.23%.
The diff coverage is 100.00%.

❗ Current head 1fca8d8 differs from pull request most recent head b2c4bab. Consider uploading reports for the commit b2c4bab to get more accurate results

@@            Coverage Diff             @@
##             main    #5602      +/-   ##
==========================================
- Coverage   91.77%   91.53%   -0.24%     
==========================================
  Files         192      192              
  Lines       11376    11456      +80     
==========================================
+ Hits        10440    10486      +46     
- Misses        742      776      +34     
  Partials      194      194

Impacted Files	Coverage Δ
pdata/internal/generated_pmetric.go	`97.38% <100.00%> (+0.02%)`	⬆️
pdata/internal/metrics.go	`80.79% <100.00%> (-11.71%)`	⬇️
pdata/internal/common.go	`92.59% <0.00%> (-2.80%)`	⬇️
service/telemetry.go	`89.20% <0.00%> (ø)`
component/exporter.go	`100.00% <0.00%> (ø)`
component/receiver.go	`100.00% <0.00%> (ø)`
component/processor.go	`100.00% <0.00%> (ø)`
config/confighttp/confighttp.go	`100.00% <0.00%> (+1.25%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b2dc376...b2c4bab. Read the comment docs.

bogdandrutu

I like it.

pdata/internal/metrics.go

TylerHelmuth · 2022-06-28T22:02:51Z

What should the strategy be to make this backwards compatible? The NewMetricDataPointFlags func change is probably the most breaking of the changes.

pdata/internal/metrics.go

bogdandrutu · 2022-06-29T09:16:36Z

What should the strategy be to make this backwards compatible? The NewMetricDataPointFlags func change is probably the most breaking of the changes.

If too hard we can do an exception, otherwise a 2 step process is preferred, add a NewEmptyMetricDataPointFlags deprecate the current one, then remove the current add the new signature and deprecate the empty.

TylerHelmuth · 2022-06-29T14:53:05Z

If too hard we can do an exception, otherwise a 2 step process is preferred, add a NewEmptyMetricDataPointFlags deprecate the current one, then remove the current add the new signature and deprecate the empty.

Done

pdata/internal/metrics_test.go

pdata/internal/metrics.go

github-actions · 2022-07-16T03:36:07Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

TylerHelmuth · 2022-07-16T04:49:06Z

Not stale, waiting for @bogdandrutu to return from PTO.

TylerHelmuth · 2022-07-25T20:16:35Z

@bogdandrutu ready for another review.

bogdandrutu

I think this is the final state that I envisioned, the problem is that we need to do this in multiple steps and deprecate things first.

pdata/pmetric/alias.go

TylerHelmuth · 2022-07-26T19:36:38Z

pdata/internal/cmd/pdatagen/internal/metrics_structs.go

@@ -489,6 +493,15 @@ var exemplar = &messageValueStruct{
 	},
 }

+var dataPointFlagsFieldStruct = &messageValueField{
+	fieldName:       "FlagsStruct",


Open to suggestions on this name.

Sounds reasonable.

The existing Flags() api doesn't actually show as deprecated when used bc it auto-generated code. Is that ok or should we flag it and point people to FlagsStruct?

I think we should mark that as deprecated, otherwise cannot expect people to change. We can do that in a separate PR.

pdata/internal/metrics.go

bogdandrutu · 2022-07-29T22:59:35Z

pdata/internal/cmd/pdatagen/internal/metrics_structs.go

@@ -489,6 +493,15 @@ var exemplar = &messageValueStruct{
 	},
 }

+var dataPointFlagsFieldStruct = &messageValueField{
+	fieldName:       "FlagsStruct",


Sounds reasonable.

CHANGELOG.md

bogdandrutu · 2022-08-01T20:40:05Z

pdata/internal/cmd/pdatagen/internal/metrics_structs.go

@@ -489,6 +493,15 @@ var exemplar = &messageValueStruct{
 	},
 }

+var dataPointFlagsFieldStruct = &messageValueField{
+	fieldName:       "FlagsStruct",


I think we should mark that as deprecated, otherwise cannot expect people to change. We can do that in a separate PR.

Update flags API

a8cc708

bogdandrutu reviewed Jun 27, 2022

View reviewed changes

pdata/internal/metrics.go Outdated Show resolved Hide resolved

Added bool to SetNoRecordedValue

dc93ae0

bogdandrutu reviewed Jun 29, 2022

View reviewed changes

pdata/internal/metrics.go Outdated Show resolved Hide resolved

Deprecate NewMetricDataPointFlags and HasFlag

ce691b0

TylerHelmuth marked this pull request as ready for review June 29, 2022 14:53

TylerHelmuth requested review from a team and dmitryax June 29, 2022 14:53

bogdandrutu reviewed Jun 29, 2022

View reviewed changes

pdata/internal/metrics_test.go Outdated Show resolved Hide resolved

pdata/internal/metrics_test.go Outdated Show resolved Hide resolved

TylerHelmuth added 4 commits June 30, 2022 13:58

Updated MetricDataPointFlags to be a struct

48642b9

Updated changelog

f160e5a

Fixed lint issues

f6b76e6

Added more tests

0cb72b6

bogdandrutu reviewed Jul 1, 2022

View reviewed changes

pdata/internal/metrics.go Outdated Show resolved Hide resolved

github-actions bot added the Stale label Jul 16, 2022

mx-psi removed the Stale label Jul 16, 2022

TylerHelmuth added 3 commits July 20, 2022 13:30

remove None and Reset

d21fab2

Merge remote-tracking branch 'upstream/main' into issue-5443

28d43a7

Merge remote-tracking branch 'upstream/main' into issue-5443

864abc0

bogdandrutu reviewed Jul 26, 2022

View reviewed changes

pdata/pmetric/alias.go Outdated Show resolved Hide resolved

Deprecate MetricDataPointFlags in favor of EmptyMetricDataPointFlags

1fca8d8

TylerHelmuth commented Jul 26, 2022

View reviewed changes

bogdandrutu reviewed Jul 29, 2022

View reviewed changes

TylerHelmuth added 2 commits August 1, 2022 09:04

Update changelog

5b5b36e

Rename EmptyMetricDataPointFlags to MetricDataPointFlagsStruct

9c53261

bogdandrutu approved these changes Aug 1, 2022

View reviewed changes

Merge branch 'main' into issue-5443

b2c4bab

bogdandrutu merged commit 3f88c04 into open-telemetry:main Aug 1, 2022

TylerHelmuth deleted the issue-5443 branch August 1, 2022 21:24

mx-psi mentioned this pull request Sep 14, 2022

[docs] Update release instructions #6068

Merged

This was referenced Sep 28, 2022

[receiver/otlp] Rewrite logs related to server startup #6174

Merged

[pdata] Update deprecation comments #6206

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update flags API #5602

Update flags API #5602

TylerHelmuth commented Jun 27, 2022

TylerHelmuth commented Jun 27, 2022

codecov bot commented Jun 27, 2022 •

edited

Loading

bogdandrutu left a comment

TylerHelmuth commented Jun 28, 2022

bogdandrutu commented Jun 29, 2022

TylerHelmuth commented Jun 29, 2022

github-actions bot commented Jul 16, 2022

TylerHelmuth commented Jul 16, 2022

TylerHelmuth commented Jul 25, 2022

bogdandrutu left a comment

TylerHelmuth Jul 26, 2022

bogdandrutu Jul 29, 2022

TylerHelmuth Jul 29, 2022

bogdandrutu Aug 1, 2022

TylerHelmuth Aug 1, 2022

bogdandrutu Jul 29, 2022

bogdandrutu Aug 1, 2022

Update flags API #5602

Update flags API #5602

Conversation

TylerHelmuth commented Jun 27, 2022

TylerHelmuth commented Jun 27, 2022

codecov bot commented Jun 27, 2022 • edited Loading

Codecov Report

bogdandrutu left a comment

Choose a reason for hiding this comment

TylerHelmuth commented Jun 28, 2022

bogdandrutu commented Jun 29, 2022

TylerHelmuth commented Jun 29, 2022

github-actions bot commented Jul 16, 2022

TylerHelmuth commented Jul 16, 2022

TylerHelmuth commented Jul 25, 2022

bogdandrutu left a comment

Choose a reason for hiding this comment

TylerHelmuth Jul 26, 2022

Choose a reason for hiding this comment

bogdandrutu Jul 29, 2022

Choose a reason for hiding this comment

TylerHelmuth Jul 29, 2022

Choose a reason for hiding this comment

bogdandrutu Aug 1, 2022

Choose a reason for hiding this comment

TylerHelmuth Aug 1, 2022

Choose a reason for hiding this comment

bogdandrutu Jul 29, 2022

Choose a reason for hiding this comment

bogdandrutu Aug 1, 2022

Choose a reason for hiding this comment

codecov bot commented Jun 27, 2022 •

edited

Loading