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

refactor: use Key's Defined method #2593

Merged
merged 3 commits into from Feb 11, 2022
Merged

Conversation

1046102779
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Feb 9, 2022

Codecov Report

Merging #2593 (eb8e591) into main (010ca4f) will not change coverage.
The diff coverage is 100.0%.

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #2593   +/-   ##
=====================================
  Coverage   76.2%   76.2%           
=====================================
  Files        173     173           
  Lines      12238   12238           
=====================================
  Hits        9326    9326           
  Misses      2669    2669           
  Partials     243     243           
Impacted Files Coverage Δ
attribute/kv.go 50.0% <100.0%> (ø)
exporters/jaeger/jaeger.go 92.6% <0.0%> (-0.9%) ⬇️
sdk/trace/batch_span_processor.go 82.1% <0.0%> (+0.9%) ⬆️

@dmathieu
Copy link
Member

You're marking this as an optimization (in your PR title). How is that so?
It seems the only reason for this change would be to reuse the key's validation, or for readability of the code.

@1046102779
Copy link
Contributor Author

You're marking this as an optimization (in your PR title). How is that so? It seems the only reason for this change would be to reuse the key's validation, or for readability of the code.

Yes. Use existing method to maintain functional consistency.

Is it necessary for this change?

@pellared
Copy link
Member

pellared commented Feb 10, 2022

@1046102779 The title is misleading. I suggest changing to refactor: use Key's Defined method

PS. I think the change would rather decrease the performance than improve it 😄 for performance-related PRs you should include benchmarks

@1046102779 1046102779 changed the title optimize(attribute): use Key's Defined method refactor: use Key's Defined method Feb 10, 2022
@1046102779
Copy link
Contributor Author

@1046102779 The title is misleading. I suggest changing to refactor: use Key's Defined method

PS. I think the change would rather decrease the performance than improve it 😄 for performance-related PRs you should include benchmarks

ok , I run the benchmark for the change.

@Aneurysm9 Aneurysm9 added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Feb 10, 2022
@Aneurysm9 Aneurysm9 merged commit b01dc21 into open-telemetry:main Feb 11, 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.

None yet

4 participants