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

Set spanmetricsprocessor call metric IsMonotonic to true #2837

Merged
merged 4 commits into from
Mar 24, 2021

Conversation

yeya24
Copy link
Contributor

@yeya24 yeya24 commented Mar 24, 2021

Signed-off-by: yeya24 yb532204897@gmail.com

Description:

As mentioned in the issue, set IsMonotonic to true.

Link to tracking Issue: #2829

Testing: Tested manually with prometheusremotewrite exporter and it works well.
Screenshot from 2021-03-23 20-34-10

Documentation:

Signed-off-by: yeya24 <yb532204897@gmail.com>
@yeya24 yeya24 requested a review from a team March 24, 2021 00:35
@codecov
Copy link

codecov bot commented Mar 24, 2021

Codecov Report

Merging #2837 (79dc7d8) into main (f9507cd) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2837      +/-   ##
==========================================
- Coverage   91.59%   91.58%   -0.02%     
==========================================
  Files         450      450              
  Lines       22189    22190       +1     
==========================================
- Hits        20325    20322       -3     
- Misses       1394     1396       +2     
- Partials      470      472       +2     
Flag Coverage Δ
integration 69.09% <ø> (ø)
unit 90.52% <100.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
processor/spanmetricsprocessor/processor.go 100.00% <100.00%> (ø)
internal/stanza/receiver.go 93.93% <0.00%> (-6.07%) ⬇️
receiver/k8sclusterreceiver/watcher.go 95.29% <0.00%> (-2.36%) ⬇️

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 f9507cd...79dc7d8. Read the comment docs.

Signed-off-by: yeya24 <yb532204897@gmail.com>
@@ -246,7 +246,8 @@ func (p *processorImp) collectCallMetrics(ilm *pdata.InstrumentationLibraryMetri

mCalls := pdata.NewMetric()
mCalls.SetDataType(pdata.MetricDataTypeIntSum)
mCalls.SetName("calls")
mCalls.SetName("calls_total")
Copy link
Contributor

Choose a reason for hiding this comment

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

You may need to update the tests to reflect the new name.

@@ -246,7 +246,8 @@ func (p *processorImp) collectCallMetrics(ilm *pdata.InstrumentationLibraryMetri

mCalls := pdata.NewMetric()
mCalls.SetDataType(pdata.MetricDataTypeIntSum)
mCalls.SetName("calls")
mCalls.SetName("calls_total")
mCalls.IntSum().SetIsMonotonic(true)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a test for this. I think it should be in verifyConsumeMetricsInput.

Signed-off-by: yeya24 <yb532204897@gmail.com>

data := m.At(mi).IntSum()
assert.Equal(t, pdata.AggregationTemporalityCumulative, data.AggregationTemporality())
assert.Equal(t, true, data.IsMonotonic())
Copy link
Contributor

Choose a reason for hiding this comment

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

I would use assert.True.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

Copy link
Contributor

@albertteoh albertteoh left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @yeya24! Not sure why the load tests are failing as these changes seem quite minimal. Is anyone able to re-run it? or maybe a commit to address my last comment will trigger another run.

Signed-off-by: yeya24 <yb532204897@gmail.com>
@tigrannajaryan tigrannajaryan merged commit 8513dec into open-telemetry:main Mar 24, 2021
pmatyjasek-sumo pushed a commit to pmatyjasek-sumo/opentelemetry-collector-contrib that referenced this pull request Apr 28, 2021
…try#2837)

Signed-off-by: yeya24 <yb532204897@gmail.com>

As mentioned in the issue, set `IsMonotonic` to true.

**Link to tracking Issue:** open-telemetry#2829

**Testing:** Tested manually with prometheusremotewrite exporter and it works well.
![Screenshot from 2021-03-23 20-34-10](https://user-images.githubusercontent.com/25150124/112236561-281c1e80-8c17-11eb-8b6c-52a1c0c4a4ab.png)
alexperez52 referenced this pull request in open-o11y/opentelemetry-collector-contrib Aug 18, 2021
* Add comment linter

For now I am excluding
- Internal packages
- Testbed packages
- Constant blocks (since the type docs is usually enough)

* Document consumer folder

* Document processor folder

* Document component folder

* Document config folder

* Document service folder

* Add nosec directive for testbed

* Document testutil folder

* Document receiver folder

* Document exporter folder

* Document cmd/schemagen folder

* Document translator folder

* Document cmd/mdatagen

* Document obsreport folder

* Remove outdated package config comment

* Update .golangci.yml

* Update .golangci.yml

Co-authored-by: Bogdan Drutu <lazy@splunk.com>
@yeya24 yeya24 deleted the update-ismonotic branch January 1, 2022 21:22
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.

4 participants