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

[exporter/signalfx] Translate metrics together instead of individually #13170

Merged
merged 9 commits into from
Aug 23, 2022

Conversation

crobert-1
Copy link
Member

@crobert-1 crobert-1 commented Aug 10, 2022

Description:

Translation rules were originally done on metrics individually. This means that only datapoints within the same metric could be operated on together. This is insufficient for the new host metrics format removing the direction dimension. To properly form metrics to match their previous format, translations must be done across metrics, especially aggregations. This only changes the fact that multiple MTS will be operated on at the same time, it doesn't change any translation rules. The solution is to do aggregations at the ScopeMetric metric level, so metrics within the same ScopeMetric can be aggregated.

This change also properly changes test metric data to be their proper format, moving metrics into corresponding ResourceMetrics and ScopeMetrics, instead of all being lumped into the same slices.

This is an internal refactoring and testing change that should not impact any input or output data. Because of this, I don't think there should be any CHANGELOG entry.

Link to tracking Issue:

This PR is the first step to solving issue #12641

Testing:

Ensured existing tests pass, data being emitted is the same as before change.

Documentation:

NA since this is a refactor without any external impact.

@crobert-1 crobert-1 requested a review from a team August 10, 2022 22:52
@crobert-1 crobert-1 requested a review from dmitryax as a code owner August 10, 2022 22:52
@dmitryax dmitryax added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Aug 11, 2022
@dmitryax
Copy link
Member

@crobert-1 PTAL at the linter failures

@crobert-1
Copy link
Member Author

crobert-1 commented Aug 11, 2022

It looks like tests are setting up resource metrics in unsupported formats, so that aggregation isn't working as expected. The extra checks would take care of this, but assuming proper metric format causes these to fail.

I'll update the test format (resource metrics should all have the same time stamp, rather than scope metrics within resource metrics having different timestamps) unless anyone disagrees with my findings.

Copy link
Member

@dmitryax dmitryax left a comment

Choose a reason for hiding this comment

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

Where is the actual change now? :)

@crobert-1 crobert-1 changed the title [exporter/signalfx] Translate metrics together instead of individually [exporter/signalfx] Update translation test data to proper format Aug 12, 2022
@crobert-1
Copy link
Member Author

Where is the actual change now? :)

The change at this point is just updating test data format. It could go with another PR that will update translation rules, but I figure it would be good to have the format change attached to the discussion we've had on this PR.

@dmitryax
Copy link
Member

but we still apply translateAndFilter on data points from one pdata.Metric, the idea is to apply it on set of data points from one pdata.ScopeMetrics object

@crobert-1
Copy link
Member Author

but we still apply translateAndFilter on data points from one pdata.Metric, the idea is to apply it on set of data points from one pdata.ScopeMetrics object

I don't know why, but I got mixed around with this review. You're right, we want to apply it at the ScopeMetrics level. For some reason I got confused and thought the inner-most loop was the scope level, rather than individual metric level.

I'll update the description again and move translateAndFilter call.

@crobert-1 crobert-1 changed the title [exporter/signalfx] Update translation test data to proper format [exporter/signalfx] Translate metrics together instead of individually Aug 12, 2022
Translation rules were originally done on metrics individually.
This means that only datapoints within the same metric could be
operated on together. This is insufficient for the new host
metrics format removing the direction dimension. To properly
form metrics to match their previous format, translations
must be done across metrics, especially aggregations.
- Remove unecessary aggregation checks
- Move translate and filter call to be for each resource metric
- Undo change to aggregation methodology
- Fix tests to properly follow metric format
Translate and filter should be done at the ScopeMetrics level. This
will allow metrics to be aggregated within a ScopeMetrics, if
translation rules modify them to be the same name.
@crobert-1 crobert-1 force-pushed the sfx_ex_combine_metrics branch from 8df8a3d to 79ed0c8 Compare August 19, 2022 22:22
@crobert-1
Copy link
Member Author

I've added a benchmark test that takes hostmetrics and times how long it takes to run MetricsToSignalFxV2. The only change to this functionality was the scope of the translation loop.

Here's the performance comparison before and after this change on the same dataset:
Before:

BenchmarkMetricConversion-12       	    2332	    504435 ns/op
BenchmarkMetricConversion-12       	    2166	    558897 ns/op
BenchmarkMetricConversion-12       	    1760	    637230 ns/op

After:

BenchmarkMetricConversion-12       	    2235	    488777 ns/op
BenchmarkMetricConversion-12       	    2451	    488565 ns/op
BenchmarkMetricConversion-12       	    2424	    514079 ns/op

I believe this shows there is a slight performance improvement for the signalfx exporter with the new functionality.

@dmitryax
Copy link
Member

@crobert-1 thanks for adding the benchmark 👍 Can you please post before and after results?

@dmitryax
Copy link
Member

Nice! Seems like there is a clear improvement. I believe we can merge it regardless of duration attribute's changes

@dmitryax dmitryax merged commit 67448b9 into open-telemetry:main Aug 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants