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

Add RemoveIf to attribute map, and rename Delete to Remove #4914

Merged
merged 4 commits into from
Mar 4, 2022

Conversation

dashpole
Copy link
Contributor

@dashpole dashpole commented Feb 23, 2022

Description:

Fixes #4756 by adding RemoveIf for AttributeMap. The implementation (and TODO) are based on the RemoveIf for AttributeValueSlice: https://github.com/open-telemetry/opentelemetry-collector/blob/model/v0.43.1/model/pdata/generated_common.go#L181

Testing:
Unit tests added.

Benchmark added to compare performance of Remove to RemoveIf when deleting a large number of elements.

$ go test -bench=.
goos: linux
goarch: amd64
pkg: go.opentelemetry.io/collector/model/pdata
cpu: Intel(R) Xeon(R) CPU @ 2.20GHz
...
BenchmarkAttributeMap_Remove-16                                  	   94230	     12897 ns/op
BenchmarkAttributeMap_RemoveIf-16                                	  374329	      3345 ns/op
...

@dashpole dashpole requested review from a team and jpkrohling February 23, 2022 22:59
@codecov
Copy link

codecov bot commented Feb 23, 2022

Codecov Report

Merging #4914 (cba5b29) into main (e0742c5) will decrease coverage by 0.01%.
The diff coverage is 93.33%.

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

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4914      +/-   ##
==========================================
- Coverage   91.04%   91.02%   -0.02%     
==========================================
  Files         178      178              
  Lines       10617    10616       -1     
==========================================
- Hits         9666     9663       -3     
- Misses        734      736       +2     
  Partials      217      217              
Impacted Files Coverage Δ
model/pdata/common.go 95.33% <93.33%> (ø)
service/collector.go 75.16% <0.00%> (-2.28%) ⬇️
model/internal/pdata/generated_metrics.go
model/internal/pdata/generated_trace.go
model/internal/pdata/traces.go
model/internal/pdata/traceid.go
model/internal/pdata/spanid.go
model/internal/pdata/generated_common.go
model/internal/pdata/timestamp.go
model/internal/pdata/metrics.go
... and 17 more

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 e0742c5...f707dca. Read the comment docs.

model/pdata/common.go Outdated Show resolved Hide resolved
@dashpole dashpole force-pushed the multiple_key_removal branch 2 times, most recently from 453002a to 95c78d0 Compare February 24, 2022 15:10
@dashpole dashpole changed the title Add DeleteIf to attribute map Add RemoveIf to attribute map, and rename Delete to Remove Feb 24, 2022
model/pdata/common.go Outdated Show resolved Hide resolved
Copy link
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, @codeboten @tigrannajaryan since this changes a bit more specifically Delete -> Remove, I want your opinion.

@dashpole dashpole force-pushed the multiple_key_removal branch from 4c0c28a to 5de260f Compare February 28, 2022 14:22
Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

No strong opinion on Delete vs Remove name. Works either way for me.

model/pdata/common.go Outdated Show resolved Hide resolved
@tigrannajaryan
Copy link
Member

No strong opinion on Delete vs Remove name. Works either way for me.

Correction: Remove/RemoveIf is preferable to be consistent with slices that have RemoveIf.

@dashpole
Copy link
Contributor Author

Leaving the comment here, since it will be easier to read than in a thread...
I think GC might actually be handling this correctly, and not leaking memory. My experimental code: dashpole/dashpole_demos@3a2c768#diff-f48531acb0ca1af513103682c6d325f99a4397db9023fa8a3bad4f4700e867a2R9

Memory usage over time:
Screen Shot 2022-02-28 at 3 15 09 PM

@tigrannajaryan
Copy link
Member

Leaving the comment here, since it will be easier to read than in a thread... I think GC might actually be handling this correctly, and not leaking memory.

Thanks for confirming!

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.

Efficient way of removing multiple keys from AttributeMap
4 participants