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.

Sign up for GitHub

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

Jump to bottom

[internal/core]: Introduce OrderMapByKey() function #34318

Closed
wants to merge 1 commit into from
Closed

[internal/core]: Introduce OrderMapByKey() function #34318

wants to merge 1 commit into from

Conversation

odubajDT
Copy link
Contributor

@odubajDT odubajDT commented Jul 30, 2024 •
edited
Loading

Description:

  • standardize order of datapoints to avoid test flakyness

@github-actions github-actions bot added cmd/otelcontribcol otelcontribcol command exporter/datadog Datadog components labels Jul 30, 2024
evan-bradley
evan-bradley reviewed Jul 30, 2024
View reviewed changes
pkg/pdatatest/pmetrictest/options.go Outdated Show resolved Hide resolved
internal/coreinternal/aggregateutil/aggregate.go Outdated
@@ -75,13 +77,13 @@ func GroupDataPoints(metric pmetric.Metric, ag *AggGroups) {
func MergeDataPoints(to pmetric.Metric, aggType AggregationType, ag AggGroups) {
switch to.Type() {
case pmetric.MetricTypeGauge:
mergeNumberDataPoints(ag.gauge, aggType, to.Gauge().DataPoints())
mergeNumberDataPoints(pmetrictest.OrderMapByKey(ag.gauge), aggType, to.Gauge().DataPoints())
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we either duplicate this functionality or include it somewhere as a non-testing library? It feels odd to rely on a test package like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved it to internal/common/maps, any better suggestions?

@odubajDT
[internal/core]: Introduce OrderMapByKey() function
833b945 
Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com>
@odubajDT odubajDT changed the title [chore]: standardize order of datapoints during aggregation [internal/core]: Introduce OrderMapByKey() function Jul 30, 2024
@odubajDT odubajDT marked this pull request as ready for review July 30, 2024 14:12
@odubajDT odubajDT requested a review from a team July 30, 2024 14:12
evan-bradley
evan-bradley reviewed Jul 30, 2024
View reviewed changes
.chloggen/order-map.yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need a changelog for this, since packages/modules under internal can't be used outside the repo (source).

internal/common/maps/maps.go
@@ -37,3 +39,22 @@ func CloneStringMap(m map[string]string) map[string]string {
}
return m2
}

func OrderMapByKey[K any](input map[string]K) map[string]K {
Copy link
Contributor

Choose a reason for hiding this comment

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

What property of the map does this affect? From what I can tell, map iteration order isn't stable, so changing the insertion order shouldn't have an impact on iteration order.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm good point, do you think we should throw out this PR then?

@odubajDT odubajDT closed this Jul 31, 2024
@odubajDT odubajDT deleted the fix-test branch July 31, 2024 06:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@evan-bradley evan-bradley evan-bradley left review comments

@djaglowski djaglowski Awaiting requested review from djaglowski

@fatsheep9146 fatsheep9146 Awaiting requested review from fatsheep9146

@ankitpatel96 ankitpatel96 Awaiting requested review from ankitpatel96

@dineshg13 dineshg13 Awaiting requested review from dineshg13

@liustanley liustanley Awaiting requested review from liustanley

@mackjmr mackjmr Awaiting requested review from mackjmr

@mx-psi mx-psi Awaiting requested review from mx-psi

@songy23 songy23 Awaiting requested review from songy23

Assignees

@mwear mwear

Labels
cmd/otelcontribcol otelcontribcol command connector/count connector/servicegraph exporter/datadog Datadog components exporter/loadbalancing internal/core internal/exp/metrics pkg/pdatatest pkg/translator/opencensus processor/attributes Attributes processor processor/deltatocumulative processor/geoip processor/interval processor/metricstransform Metrics Transform processor processor/resource Resource processor receiver/activedirectoryds receiver/aerospike receiver/apache receiver/apachespark receiver/azuremonitor receiver/bigip receiver/couchdb receiver/dockerstats receiver/elasticsearch receiver/expvar receiver/filestats receiver/flinkmetrics receiver/gitprovider receiver/haproxy receiver/hostmetrics receiver/httpcheck HTTP Check receiver receiver/iis receiver/memcached receiver/mongodb receiver/mysql receiver/nginx receiver/nsxt NSXT Receiver receiver/rabbitmq receiver/receivercreator receiver/redis Redis related issues receiver/riak receiver/saphana receiver/snmp receiver/snowflake Snowflake receiver receiver/splunkenterprise receiver/sqlquery SQL query receiver receiver/sqlserver receiver/sshcheck receiver/vcenter receiver/windowsperfcounters
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@odubajDT @evan-bradley @mwear