Skip to content

Move InstrumentationLibrary key generation logic out of TracerProvider/SpanConverter [#619]#663

Merged
tidal merged 5 commits into
open-telemetry:mainfrom
ditsuke:refactor-instrumentation-lib-key-gen
Apr 15, 2022
Merged

Move InstrumentationLibrary key generation logic out of TracerProvider/SpanConverter [#619]#663
tidal merged 5 commits into
open-telemetry:mainfrom
ditsuke:refactor-instrumentation-lib-key-gen

Conversation

@ditsuke

@ditsuke ditsuke commented Apr 14, 2022

Copy link
Copy Markdown
Contributor

PR for Issue #619.

Factors key generation logic for InstrumentationLibrary instances out to a helper method in a utils class, replacing generation in TracerProvider and SpanConverter with calls to the new method.

@ditsuke
Extract InstrumentationLibrary key generation logic
0b6eea6 
Factors key generation logic for InstrumentationLibrary instances
out to a helper method in a utils class.
@welcome

welcome Bot commented Apr 14, 2022

Copy link
Copy Markdown

Thanks for opening your first pull request! If you haven't yet signed our Contributor License Agreement (CLA), then please do so that we can accept your contribution. A link should appear shortly in this PR if you have not already signed one.

@ditsuke

ditsuke commented Apr 14, 2022

Copy link
Copy Markdown
Contributor Author

Could use some feedback on the new Helpers class, can we do a better name?

@ditsuke
Remove @see PHP annotation
c060eb1 
Phan doesn't like it, couldn't see an acceptable workaround.
@brettmc

brettmc commented Apr 15, 2022

Copy link
Copy Markdown
Contributor

@ditsuke welcome aboard! re: class naming, I think SDK\Common\Instrumentation is a better location. As to class name, KeyGenerator? It will also need unit tests.

@codecov

codecov Bot commented Apr 15, 2022 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #663 (0f3665e) into main (7ce599b) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #663      +/-   ##
============================================
+ Coverage     84.27%   84.28%   +0.01%     
- Complexity     1152     1153       +1     
============================================
  Files           127      128       +1     
  Lines          2786     2788       +2     
============================================
+ Hits           2348     2350       +2     
  Misses          438      438
Flag Coverage Δ
7.4 84.23% <100.00%> (+0.01%) ⬆️
8.0 84.28% <100.00%> (+0.01%) ⬆️
8.1 84.28% <100.00%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
src/Contrib/Otlp/SpanConverter.php 100.00% <100.00%> (ø)
src/SDK/Common/Instrumentation/KeyGenerator.php 100.00% <100.00%> (ø)
src/SDK/Trace/TracerProvider.php 100.00% <100.00%> (ø)

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 7ce599b...0f3665e. Read the comment docs.

ditsuke added 2 commits April 15, 2022 12:09
@ditsuke
Change key generator namespace and class
0b9cb26 
Changes the key generator's namespace to Util\Instrumentation and classname to
KeyGenerator, refactoring usages.
@ditsuke
Add unit tests for Instrumentation key generator
19ff4cc
@ditsuke

ditsuke commented Apr 15, 2022

Copy link
Copy Markdown
Contributor Author

@ditsuke welcome aboard! re: class naming, I think SDK\Common\Instrumentation is a better location. As to class name, KeyGenerator? It will also need unit tests.

@brettmc thanks! I've moved and renamed the generator and added unit tests, PTAL

tidal
tidal reviewed Apr 15, 2022
View reviewed changes
Comment thread tests/Unit/SDK/Common/Instrumentation/KeyGeneratorTest.php
@tidal

tidal commented Apr 15, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

@ditsuke
Thank you. LGTM
I have added one little comment for a test you could add, just to make sure.
However other than t, I think we can merge the PR, if you are ok with it.

@tidal tidal linked an issue Apr 15, 2022 that may be closed by this pull request
Move InstrumentationLibrary key generation out of TracerProvider #619
Closed
@ditsuke

ditsuke commented Apr 15, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@ditsuke Thank you. LGTM I have added one little comment for a test you could add, just to make sure. However other than t, I think we can merge the PR, if you are ok with it.

@tidal thanks for reviewing. I've added the third test with 0f3665e

@ditsuke
ditsuke force-pushed the refactor-instrumentation-lib-key-gen branch from b84e3bb to 0f3665e Compare April 15, 2022 12:12
@ditsuke
Add test case for non-null version and null schema
0f3665e 
Also renames the other 2 tests for descriptiveness.
@tidal
tidal merged commit 6d99358 into open-telemetry:main Apr 15, 2022
@ditsuke

ditsuke commented Apr 15, 2022

Copy link
Copy Markdown
Contributor Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bobstrecansky bobstrecansky Awaiting requested review from bobstrecansky
@zsistla zsistla Awaiting requested review from zsistla
@brettmc brettmc Awaiting requested review from brettmc
1 more reviewer
@tidal tidal tidal approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Move InstrumentationLibrary key generation out of TracerProvider

3 participants

@ditsuke @brettmc @tidal