Skip to content

rename DoubleSum -> Sum#3583

Merged
bogdandrutu merged 5 commits intoopen-telemetry:mainfrom
codeboten:codeboten/rename-doublesum
Jul 12, 2021
Merged

rename DoubleSum -> Sum#3583
bogdandrutu merged 5 commits intoopen-telemetry:mainfrom
codeboten:codeboten/rename-doublesum

Conversation

@codeboten
Copy link
Copy Markdown
Contributor

Description:
Continuing the work started in updating OTLP to v0.8.0, DoubleSum is now Sum

Link to tracking Issue: Part of #3534

@codeboten codeboten requested a review from alolita as a code owner July 8, 2021 22:22
@codeboten codeboten requested review from a team and dmitryax July 8, 2021 22:22
Copy link
Copy Markdown
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

I think we should split this:

  1. Changes in cmd/mdatagen
  2. Changes in pdata (and their usages)
  3. Changes in internal/goldendata

If possible :)

@bogdandrutu
Copy link
Copy Markdown
Member

After looking at the PR only changes in the internal/goldendata about the pict files worries me a bit, if these can be separated and discuss more we can move forward with what we have here

@codeboten
Copy link
Copy Markdown
Contributor Author

After looking at the PR only changes in the internal/goldendata about the pict files worries me a bit, if these can be separated and discuss more we can move forward with what we have here

Happy to remove the changes to pict files, are there steps to regenerate them automatically documented anywhere?

@bogdandrutu
Copy link
Copy Markdown
Member

@codeboten I am useless here, see https://github.com/open-telemetry/opentelemetry-collector/issues/3378 where I asked the same :))

@codeboten
Copy link
Copy Markdown
Contributor Author

fwiw, I ran

pict ./testdata/pict_input_metrics.txt > testdata/generated_pict_pairs_metrics.txt 

With the data in this PR and the output matched this PR. Note that there is an existing discrepancy in the data fixed in #3587

cfg.MetricDescriptorType = pdata.MetricDataTypeDoubleGauge
case MetricTypeMonotonicDoubleSum:
cfg.MetricDescriptorType = pdata.MetricDataTypeDoubleSum
case MetricTypeMonotonicSum:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ok my point was that I don't think we should do this change since, the Sum will have 2 possible values (Int or Double). So for this is good to keep Double in the name to signal that Sum will have Double values in the NumberDataPoint.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah I see. Thanks for clarifying!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

pict changes reverted.

@bogdandrutu
Copy link
Copy Markdown
Member

Unfortunately I cannot merge this because the upgrade of the contrib for otel-trace changes is blocked. To not waste time let's start address other issues in the meantime.

One PR that we can do is to have the conversion of IntHistogram to Histogram. We need to call this code every time when we unmarshal metrics so all the components will be able to ignore IntHistograms and we can delete them.

I am proposing this PR until we unblock the contrib upgrade. What do you think?

Copy link
Copy Markdown
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

LGTM but blocking because of contrib not being able to be upgraded because of otel-trace issues.

@codeboten
Copy link
Copy Markdown
Contributor Author

One PR that we can do is to have the conversion of IntHistogram to Histogram. We need to call this code every time when we unmarshal metrics so all the components will be able to ignore IntHistograms and we can delete them.

Sounds like a good next step @bogdandrutu, I've also got the PRs to rename DoubleGauge ready to go once the DoubleSum PRs are merged.

@bogdandrutu bogdandrutu merged commit aa60edf into open-telemetry:main Jul 12, 2021
@codeboten codeboten deleted the codeboten/rename-doublesum branch June 22, 2023 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants