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

[extension/googleclientauth] Add GCP auth extension implementation #32029

Merged
merged 12 commits into from
Apr 15, 2024

Conversation

damemi
Copy link
Contributor

@damemi damemi commented Mar 28, 2024

Description:
Follow up to boilerplate PR #31518
This adds the functions calls to actually implement the extension using our downstream libraries.

It also enables the shutdown tests for this extension (lifecycle tests do not seem possible for us currently)

Link to tracking Issue: n/a

Testing: downstream unit and integration tests, shutdown tests here

Documentation: Here and downstream READMEs

Copy link
Contributor

@dashpole dashpole left a comment

Choose a reason for hiding this comment

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

Actually, can you add tests for the config? Usually, that involves a testdata directory with example configs. Like this one: https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/exporter/googlecloudexporter/config_test.go.

It would also be good to check for leaked goroutines: https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/exporter/googlecloudexporter/package_test.go

@dashpole dashpole self-requested a review April 1, 2024 13:42
@damemi
Copy link
Contributor Author

damemi commented Apr 1, 2024

@dashpole sure, I'll add config tests. We do have the leaked goroutines test already https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/e867aa84edb2dbe0570b57681ba62bdbf92e1d37/extension/googleclientauthextension/package_test.go (is there anything we need to add to that?)

@dashpole
Copy link
Contributor

dashpole commented Apr 1, 2024

Nope, nothing else there. I just missed them when I looked

@damemi
Copy link
Contributor Author

damemi commented Apr 1, 2024

@dashpole I updated to add config tests, but double check my pointer logic in aa8288f

For this extension we return a pointer default config downstream https://github.com/GoogleCloudPlatform/opentelemetry-operations-go/blob/4caace7de8789f91dc3ddcdbcd328db002c44abb/extension/googleclientauthextension/config.go#L58

But for our exporters, it's not a pointer: https://github.com/GoogleCloudPlatform/opentelemetry-operations-go/blob/4caace7de8789f91dc3ddcdbcd328db002c44abb/exporter/collector/config.go#L198

Not sure if this is intentional, but I want to make sure this won't cause a nil pointer in the config. The code is also kind of weird. The alternative is another release downstream to not use a pointer when we call DefaultConfig, then I think the embedded type should just work

@dashpole
Copy link
Contributor

dashpole commented Apr 1, 2024

Why did you need to add the pointer?

@dashpole dashpole added the ready to merge Code review completed; ready to merge by maintainers label Apr 1, 2024
@damemi
Copy link
Contributor Author

damemi commented Apr 1, 2024

@dashpole you're right, I could have dereferenced it and think I was misreading the error that I was seeing. I didn't save it, but the issue was with a conflict in the upstream and downstream Config types. I think we should refactor the downstream extension code to not rely on the component.Component interface if possible and just use a downstream struct. That's closer to what our exporters do. But for now this should work

@dashpole dashpole removed the ready to merge Code review completed; ready to merge by maintainers label Apr 8, 2024
@damemi
Copy link
Contributor Author

damemi commented Apr 8, 2024

Fixed conflicts, @dashpole this is still ready to merge

@dashpole dashpole added the ready to merge Code review completed; ready to merge by maintainers label Apr 8, 2024
@damemi
Copy link
Contributor Author

damemi commented Apr 15, 2024

@djaglowski hi, since you merged the last PR for this component I wanted to see if this was ready to merge? It's been a couple weeks with the ready-to-merge label set and it's tough to keep up with the merge conflicts that keep popping up. Would like to nudge this if possible, thanks!

@djaglowski djaglowski merged commit 7608ecb into open-telemetry:main Apr 15, 2024
170 checks passed
@github-actions github-actions bot added this to the next release milestone Apr 15, 2024
@djaglowski
Copy link
Member

@damemi, thanks for the ping. Always feel free to ping me or any other maintainer as soon as an approver marks it ready to merge.

djaglowski pushed a commit that referenced this pull request Apr 18, 2024
…2442)

**Description:** <Describe what has changed.>
Follow up to
#32029
and
#31518
This marks the `googleclientauthextension` as `alpha` following the
[contributing
doc](https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/CONTRIBUTING.md#adding-new-components)
and [alpha stability
guidelines](https://github.com/open-telemetry/opentelemetry-collector#alpha).
It also adds it to `otelcontribcol`

Note this also updates CONTRIBUTING.md to explain how to add the alpha
component to otelcontribcol.

**Link to tracking Issue:** N/A

**Testing:** Downstream unit and integration tests

**Documentation:** No changes except stability
rimitchell pushed a commit to rimitchell/opentelemetry-collector-contrib that referenced this pull request May 8, 2024
…pen-telemetry#32029)

**Description:**
Follow up to boilerplate PR
open-telemetry#31518
This adds the functions calls to actually implement the extension using
our downstream libraries.

It also enables the shutdown tests for this extension (lifecycle tests
do not seem possible for us currently)

**Link to tracking Issue:** n/a

**Testing:** downstream unit and integration tests, shutdown tests here

**Documentation:** Here and downstream READMEs
rimitchell pushed a commit to rimitchell/opentelemetry-collector-contrib that referenced this pull request May 8, 2024
…en-telemetry#32442)

**Description:** <Describe what has changed.>
Follow up to
open-telemetry#32029
and
open-telemetry#31518
This marks the `googleclientauthextension` as `alpha` following the
[contributing
doc](https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/CONTRIBUTING.md#adding-new-components)
and [alpha stability
guidelines](https://github.com/open-telemetry/opentelemetry-collector#alpha).
It also adds it to `otelcontribcol`

Note this also updates CONTRIBUTING.md to explain how to add the alpha
component to otelcontribcol.

**Link to tracking Issue:** N/A

**Testing:** Downstream unit and integration tests

**Documentation:** No changes except stability
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extension/googleclientauth ready to merge Code review completed; ready to merge by maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants