Updated configgrpc ToDialOptions and confighttp ToClient apis to take extensions configuration map #3340

pavankrish123 · 2021-05-31T02:24:45Z

This PR is a port of the configfrpc's ToDialOptions() and confighttp ToClient() from PR #3128 in a piece meal fashion.

The following are the changes

Refactored configgrpc.PerRPCAuth as extension implementing configauth.GrpcClientAuthenticator
Plugged in extensions configuration to all the grpc based clients in the core (OTLP, OpenCensus, Jaeger, JaegerReceiver)
Plugged in extensions configuration to all the HTTP based clients in the core (Zipkin, OTLPHTTP)

Link to tracking Issue:
#3282 #3276

Testing:
Unit tests, manual test described (for only oidc)

pavankrish123 · 2021-05-31T22:02:14Z

cc: @bogdandrutu @jpkrohling @gramidt @tigrannajaryan

These changes will break the contrib and need updates to clients on contrib side in the similar fashion.

jpkrohling

This is mainly the code that has been reviewed before, right? It looks good to me, just found a couple of minor issues.

I left a comment about whether we'd want to keep backward compatibility for the PerRPCAuth option. I'll let you decide on that, and it's OK to break compatibility at this phase. In fact, it's commonly done by other components. The argument I have for doing this change in a backward compatible manner is to build the mindset that will soon be required for such changes.

config/configgrpc/configgrpc.go

config/confighttp/confighttp.go

extension/bearertokenauthextension/README.md

jpkrohling · 2021-06-02T10:18:37Z

I think this should be merged only when we have a PR ready to change the contrib components that will be broken by this. That other PR can then be updated to reference the latest core once this PR here gets merged.

gramidt · 2021-06-02T14:09:25Z

@jpkrohling @pavankrish123 - I'll have a PR ready to be rebased when we're ready. This should be straightforward given that all related PRs of open-telemetry/opentelemetry-collector-contrib#3507 have been merged.

@jpkrohling - While I agree with the backwards compatible mindset for the 1.0 release, I do not believe it's fair that we ask for this after we all approved the prior PRs, previously merged these changes, and we're still pre-1.0.

Co-authored-by: Juraci Paixão Kröhling <[email protected]>

pavankrish123 · 2021-06-02T15:51:10Z

Thanks @gramidt, I will keep the PR as is. Let me know when we are done from contrib. I have updated the README suggestions from @jpkrohling.

Co-authored-by: Juraci Paixão Kröhling <[email protected]>

gramidt · 2021-06-02T15:53:47Z

That's excellent news, @pavankrish123!

@jpkrohling @pavankrish123 - I have started the the changes needed for contrib in open-telemetry/opentelemetry-collector-contrib#3694; however, I'll need to wait until this is merged to build, run tests, and fix any syntax errors I had while adding method calls that weren't valid without these changes.

…collector into api_changes

gramidt · 2021-06-02T19:17:11Z

That's excellent news, @pavankrish123!

@jpkrohling @pavankrish123 - I have started the the changes needed for contrib in open-telemetry/opentelemetry-collector-contrib#3694; however, I'll need to wait until this is merged to build, run tests, and fix any syntax errors I had while adding method calls that weren't valid without these changes.

cc @bogdandrutu @tigrannajaryan

jpkrohling · 2021-06-03T07:15:23Z

Re-running tests, as the load test failed due to a known intermittent test failure.

tigrannajaryan · 2021-06-03T13:48:04Z

@pavankrish123 please rebase from main. I want to make sure the only failures on contrib-tests job are coming from the changes in this PR.

@gramidt to confirm open-telemetry/opentelemetry-collector-contrib#3694 is lined up to be merged immediately once this PR is merged, right? You will only need to update the core version to latest, correct?

tigrannajaryan · 2021-06-03T13:48:49Z

Re-running tests, as the load test failed due to a known intermittent test failure.

@jpkrohling Can you please record a bug or if it needs an increase in resource limits make that increase?

jpkrohling · 2021-06-03T13:50:38Z

There's one issue already, and I'll send a PR soon to increase the limits.

#2911

tigrannajaryan

LGTM.
@bogdandrutu any concerns? I believe this moves in the desired smaller increment now and is ready to be merged.

gramidt · 2021-06-03T13:52:02Z

@pavankrish123 please rebase from main. I want to make sure the only failures on contrib-tests job are coming from the changes in this PR.

@gramidt to confirm open-telemetry/opentelemetry-collector-contrib#3694 is lined up to be merged immediately once this PR is merged, right? You will only need to update the core version to latest, correct?

That is the plan; however, i will not know until I'm able to update core, since I added code that is not valid until this is merged and core is updated. If there's other core changes that require additional work, I can not speak to that effort. I will also be out of the office for another 3 hrs, but can work on it immediately once I return.

jpkrohling · 2021-06-03T14:23:38Z

@gramidt there's one way to do it (examples using gh CLI tool)

cd github.com/open-telemetry/opentelemetry-collector
$ gh pr checkout 3340
cd github.com/open-telemetry/opentelemetry-collector-contrib
$ gh pr checkout 3694
for file in $(find . -name go.mod); do go mod edit -replace go.opentelemetry.io/[email protected]=/home/jpkroehling/Projects/src/github.com/open-telemetry/opentelemetry-collector $file ; done (replace the local path on this last one)

I just did that and make test passed. make all also completed without errors.

jpkrohling · 2021-06-03T14:24:15Z

With your permission, @gramidt, I'm updating this PR with the rebase I did locally to test the commands I mentioned earlier.

jpkrohling · 2021-06-03T14:25:38Z

Actually, looks like I can't update this PR, as I'm not a maintainer :-) Rebasing to the latest main caused only one conflict, which should be easy to solve.

gramidt · 2021-06-03T15:37:36Z

Thanks @jpkrohling! I'll be back home sooner than originally planned, so I should be able to complete it relatively soon. Also, I can't believe I spaced out and forgot about using my local core collector. Haha

pavankrish123 · 2021-06-03T16:11:15Z

Rebased the master cc: @jpkrohling @gramidt @tigrannajaryan

tigrannajaryan · 2021-06-03T18:22:54Z

This is good to go, merging.

pavankrish123 · 2021-06-03T18:23:53Z

Thanks @tigrannajaryan - @gramidt we are all set to commit on contrib

tigrannajaryan · 2021-06-03T18:24:00Z

Thanks @pavankrish123 !

…ication Extensions (2/2) (#3694) This is the second phase of #3507. **Link to tracking Issue:** #3692 Component changes are based on open-telemetry/opentelemetry-collector#3340. Also related to open-telemetry/opentelemetry-collector#3282

pavankrish123 added 5 commits May 28, 2021 15:34

changes so far

c83c72a

Refactor opencensus exporter for upcoming API changes

c1f224d

fixed tests

3e26155

Merge branch 'refactor/opencensus' into api_changes

a679b49

made changes to grpc ToDialOptions

8f861f8

pavankrish123 added 5 commits May 30, 2021 19:45

missed jaeger

ce87a11

missed jaeger

bc213ab

Merge branch 'master' into api_changes

db7ff5a

Added bearertoken files

fb3c5a9

Fixed imports group

fec9c0a

pavankrish123 marked this pull request as ready for review May 31, 2021 18:55

pavankrish123 requested a review from a team May 31, 2021 18:55

pavankrish123 changed the title ~~[Draft] Updated configgRPC ToDialOptions to take extensions configuration map~~ Updated configgRPC ToDialOptions to take extensions configuration map May 31, 2021

updated confighttp ToClient() apis and corresponding exporters

7c67d6d

pavankrish123 changed the title ~~Updated configgRPC ToDialOptions to take extensions configuration map~~ Updated configgRPC ToDialOptions and confighttp ToClient apis to take extensions configuration map May 31, 2021

pavankrish123 changed the title ~~Updated configgRPC ToDialOptions and confighttp ToClient apis to take extensions configuration map~~ Updated configgrpc ToDialOptions and confighttp ToClient apis to take extensions configuration map May 31, 2021

jpkrohling reviewed Jun 2, 2021

View reviewed changes

gramidt mentioned this pull request Jun 2, 2021

Configure components for the Client Authentication Extensions (2/2) open-telemetry/opentelemetry-collector-contrib#3692

Closed

Update config/confighttp/confighttp.go

3a7fe63

Co-authored-by: Juraci Paixão Kröhling <[email protected]>

pavankrish123 requested a review from alolita as a code owner June 2, 2021 15:48

pavankrish123 and others added 2 commits June 2, 2021 08:48

Update extension/bearertokenauthextension/README.md

740bccf

Co-authored-by: Juraci Paixão Kröhling <[email protected]>

Update extension/bearertokenauthextension/README.md

42edc62

Co-authored-by: Juraci Paixão Kröhling <[email protected]>

gramidt mentioned this pull request Jun 2, 2021

Update core to latest and configure components for the Client Authentication Extensions (2/2) open-telemetry/opentelemetry-collector-contrib#3694

Merged

Update extension/bearertokenauthextension/README.md

20abec7

Co-authored-by: Juraci Paixão Kröhling <[email protected]>

pavankrish123 added 4 commits June 2, 2021 09:00

Merge branch 'master' into api_changes

9e4177b

updated per new core

9e63745

Merge branch 'api_changes' of https://github.com/cisco/opentelemetry-…

13f5146

…collector into api_changes

make fmt

79cf7af

gramidt approved these changes Jun 2, 2021

View reviewed changes

jpkrohling approved these changes Jun 3, 2021

View reviewed changes

tigrannajaryan approved these changes Jun 3, 2021

View reviewed changes

pavankrish123 added 3 commits June 3, 2021 09:12

Merge branch 'master' into api_changes

07cf58e

fixed readme

fbb2f5d

fixed readme

8993d13

tigrannajaryan merged commit 5369d7e into open-telemetry:main Jun 3, 2021

pavankrish123 deleted the api_changes branch June 8, 2021 04:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated configgrpc ToDialOptions and confighttp ToClient apis to take extensions configuration map #3340

Updated configgrpc ToDialOptions and confighttp ToClient apis to take extensions configuration map #3340

pavankrish123 commented May 31, 2021 •

edited

Loading

pavankrish123 commented May 31, 2021 •

edited

Loading

jpkrohling left a comment

jpkrohling commented Jun 2, 2021

gramidt commented Jun 2, 2021 •

edited

Loading

pavankrish123 commented Jun 2, 2021

gramidt commented Jun 2, 2021 •

edited

Loading

gramidt commented Jun 2, 2021

jpkrohling commented Jun 3, 2021

tigrannajaryan commented Jun 3, 2021

tigrannajaryan commented Jun 3, 2021

jpkrohling commented Jun 3, 2021

tigrannajaryan left a comment

gramidt commented Jun 3, 2021

jpkrohling commented Jun 3, 2021

jpkrohling commented Jun 3, 2021

jpkrohling commented Jun 3, 2021

gramidt commented Jun 3, 2021

pavankrish123 commented Jun 3, 2021 •

edited

Loading

tigrannajaryan commented Jun 3, 2021

pavankrish123 commented Jun 3, 2021

tigrannajaryan commented Jun 3, 2021

Updated configgrpc ToDialOptions and confighttp ToClient apis to take extensions configuration map #3340

Updated configgrpc ToDialOptions and confighttp ToClient apis to take extensions configuration map #3340

Conversation

pavankrish123 commented May 31, 2021 • edited Loading

pavankrish123 commented May 31, 2021 • edited Loading

jpkrohling left a comment

Choose a reason for hiding this comment

jpkrohling commented Jun 2, 2021

gramidt commented Jun 2, 2021 • edited Loading

pavankrish123 commented Jun 2, 2021

gramidt commented Jun 2, 2021 • edited Loading

gramidt commented Jun 2, 2021

jpkrohling commented Jun 3, 2021

tigrannajaryan commented Jun 3, 2021

tigrannajaryan commented Jun 3, 2021

jpkrohling commented Jun 3, 2021

tigrannajaryan left a comment

Choose a reason for hiding this comment

gramidt commented Jun 3, 2021

jpkrohling commented Jun 3, 2021

jpkrohling commented Jun 3, 2021

jpkrohling commented Jun 3, 2021

gramidt commented Jun 3, 2021

pavankrish123 commented Jun 3, 2021 • edited Loading

tigrannajaryan commented Jun 3, 2021

pavankrish123 commented Jun 3, 2021

tigrannajaryan commented Jun 3, 2021

pavankrish123 commented May 31, 2021 •

edited

Loading

pavankrish123 commented May 31, 2021 •

edited

Loading

gramidt commented Jun 2, 2021 •

edited

Loading

gramidt commented Jun 2, 2021 •

edited

Loading

pavankrish123 commented Jun 3, 2021 •

edited

Loading