Skip to content

Add endpoint.peer.service.mapping property to allow a user to specify…#562

Merged
anuraaga merged 11 commits into
open-telemetry:masterfrom
anuraaga:config-map-property
Jun 29, 2020
Merged

Add endpoint.peer.service.mapping property to allow a user to specify…#562
anuraaga merged 11 commits into
open-telemetry:masterfrom
anuraaga:config-map-property

Conversation

@anuraaga

@anuraaga anuraaga commented Jun 23, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

… mappings from an endpoint to a service name and use it to map exported spans to a service name.

For #555

Add endpoint.peer.service.mapping property to allow a user to specify…
9cebad9 
… mappings from an endpoint to a service name.
@anuraaga

anuraaga commented Jun 24, 2020

Copy link
Copy Markdown
Contributor Author

I went ahead and added the business logic too in case it helps with more context (it probably does for another issue I'm working on anyways :) ).

@anuraaga anuraaga force-pushed the config-map-property branch from 1e9947e to e88fdae Compare June 25, 2020 00:45
@anuraaga anuraaga force-pushed the config-map-property branch from 2cef348 to 281d9c4 Compare June 26, 2020 07:38
@anuraaga

anuraaga commented Jun 26, 2020

Copy link
Copy Markdown
Contributor Author

Chatted with @trask and he suggested that we move the logic into a decorator and try it out.

@anuraaga anuraaga mentioned this pull request Jun 26, 2020
Merged
trask
trask approved these changes Jun 26, 2020
View reviewed changes

@trask trask left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i think this is a good initial implementation of open-telemetry/opentelemetry-specification#652

@trask

trask commented Jun 27, 2020

Copy link
Copy Markdown
Member

@anuraaga can u check the test failures? looks like they are of the non-sporadic variety 😄

@anuraaga

anuraaga commented Jun 28, 2020

Copy link
Copy Markdown
Contributor Author

@trask Might need some help, have stared a while but can't for the life of me figure out what's wrong with the tests for the subclass of BaseDecoratorTest.

Step debugging in IntelliJ the assertions seem to work, but the unit test output from Gradle looks strange.

BaseDecoratorTest - has 4 executions of each of the onPeerConnection tests

Test | Duration | Result
-- | -- | --
test afterStart | 0.001s | passed
test assert null span | 0.001s | passed
test beforeFinish | 0s | passed
test onError[0] | 0s | passed
test onError[1] | 0s | passed
test onPeerConnection with mapped peer[0] | 0.001s | passed
test onPeerConnection with mapped peer[1] | 0s | passed
test onPeerConnection with mapped peer[2] | 0s | passed
test onPeerConnection with mapped peer[3] | 0s | passed
test onPeerConnection[0] | 0.001s | passed
test onPeerConnection[1] | 0.001s | passed
test onPeerConnection[2] | 0.001s | passed
test onPeerConnection[3] | 0s | passed
test spanNameForMethod[0] | 0s | passed
test spanNameForMethod[1] | 0s | passed
test spanNameForMethod[2] | 0s | passed

ClientDecoratorTest - Only has one


Test | Duration | Result
-- | -- | --
test afterStart | 0.036s | passed
test afterStart[0] | 0.002s | passed
test afterStart[1] | 0.001s | passed
test afterStart[2] | 0s | passed
test assert null span | 0.001s | passed
test beforeFinish | 0s | passed
test beforeFinish | 0.001s | passed
test currentContextWith | 0.001s | passed
test getOrCreateSpan internal after client span | 0.012s | passed
test getOrCreateSpan when existing client span | 0.008s | passed
test getOrCreateSpan when no existing client span | 0.017s | passed
test onError | 0.009s | passed
test onPeerConnection | 5.186s | passed
test onPeerConnection with mapped peer | 0.029s | failed
test spanNameForMethod | 0.031s | passed

Any pointers to what might be missing? It seems like an interaction of spock + subclass but not sure.

@trask

trask commented Jun 28, 2020

Copy link
Copy Markdown
Member

hey, looks like there's a span field in the subclasses also, and for some reason (???) if you reference span in the base class inside of a closure, it picks up the span from the subclass:

    ConfigUtils.withConfigOverride(
      "endpoint.peer.service.mapping",
      "1.2.3.4=catservice,dogs.com=dogsservice,opentelemetry.io=specservice") {
      decorator.onPeerConnection(span, connection)
    }

anyways, delete the span field in the subclasses i think will resolve (and keep others from hitting this)

@anuraaga

anuraaga commented Jun 28, 2020

Copy link
Copy Markdown
Contributor Author

Thanks - crazy behavior, and crazy debugging skills!

@anuraaga anuraaga merged commit ca0d676 into open-telemetry:master Jun 29, 2020
@iNikem iNikem mentioned this pull request Jul 9, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@trask trask trask approved these changes

@iNikem iNikem Awaiting requested review from iNikem

@jkwatson jkwatson Awaiting requested review from jkwatson

@tylerbenson tylerbenson Awaiting requested review from tylerbenson

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@anuraaga @trask