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

[receiver/googlecloudmonitoringreceiver] Adding Google Cloud monitoring receiver #34015

Conversation

abhishek-at-cloudwerx
Copy link
Contributor

Description: Adding Google Cloud Monitoring receiver

Link to tracking Issue: #33762

Testing: Test cases for configuration added

Documentation: README file added for describing the component

Copy link

linux-foundation-easycla bot commented Jul 10, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@crobert-1
Copy link
Member

Hello @abhishek-at-cloudwerx, please sign the CLA to enable this PR to be reviewed.

@abhishek-at-cloudwerx
Copy link
Contributor Author

@crobert-1 - Yes we're working on getting the org CLA sorted. Unfortunately we are unable to get any help in figuring out who registered as Org admin.

@abhishek-at-cloudwerx abhishek-at-cloudwerx marked this pull request as ready for review July 19, 2024 05:47
@abhishek-at-cloudwerx abhishek-at-cloudwerx requested review from a team and djaglowski July 19, 2024 05:47
@abhishek-at-cloudwerx
Copy link
Contributor Author

@crobert-1 - The CLA is now signed and PR is ready for review.

@dashpole - Tagging as Sponsor
@TylerHelmuth - to be one of the CODEOWNERS

.chloggen/googlecloudmonitoringreceiver-phase1.yaml Outdated Show resolved Hide resolved
cmd/otelcontribcol/go.mod Outdated Show resolved Hide resolved
receiver/googlecloudmonitoringreceiver/receiver.go Outdated Show resolved Hide resolved
receiver/googlecloudmonitoringreceiver/config.go Outdated Show resolved Hide resolved
receiver/googlecloudmonitoringreceiver/config.go Outdated Show resolved Hide resolved
receiver/googlecloudmonitoringreceiver/config.go Outdated Show resolved Hide resolved
receiver/googlecloudmonitoringreceiver/config.go Outdated Show resolved Hide resolved
@abhishek-at-cloudwerx
Copy link
Contributor Author

@dashpole I made the changes as per your comment and removed the service_name field since we now fetch details directly based on the metric_name.

cmd/otelcontribcol/go.mod Outdated Show resolved Hide resolved
cmd/oteltestbedcol/go.mod Outdated Show resolved Hide resolved
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.

lgtm after nits addressed

Copy link
Contributor

@atoulme atoulme left a comment

Choose a reason for hiding this comment

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

LGTM after nits addressed, @TylerHelmuth should review as well

@abhishek-at-cloudwerx
Copy link
Contributor Author

@TylerHelmuth @dashpole - Could one of you help merge this ?
Thanks

Copy link
Member

@TylerHelmuth TylerHelmuth left a comment

Choose a reason for hiding this comment

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

Please take a look at the failing CI

@abhishek-at-cloudwerx
Copy link
Contributor Author

Thanks @TylerHelmuth for approval. @djaglowski could you please help with the final approval and merge as CODEOWNER.

Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

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

@abhishek-at-cloudwerx LGTM but I'm not a codeowner on this component.

@dashpole dashpole added the ready to merge Code review completed; ready to merge by maintainers label Aug 7, 2024
@dashpole
Copy link
Contributor

dashpole commented Aug 9, 2024

Still some presubmit failures

@abhishek-at-cloudwerx
Copy link
Contributor Author

Still some presubmit failures

@dashpole - Yes we're currently reviewing those and will drop a note once all is in good shape.

@dashpole dashpole added the ready to merge Code review completed; ready to merge by maintainers label Aug 12, 2024
@codeboten codeboten merged commit 2515c52 into open-telemetry:main Aug 12, 2024
161 checks passed
@github-actions github-actions bot added this to the next release milestone Aug 12, 2024
dmitryax pushed a commit that referenced this pull request Aug 12, 2024
**Description:**
As a result of
#34585,
#34015,
and
#34572,
we've had some inconsistencies and incorrect information introduced
around code owners. This corrects these errors by updating
`metadata.yaml` to match the `CODEOWNERS` file, as well as updating
`CODEOWNERS` format, and adding a user to the `allowlist` of code owners
that are not OpenTelemetry members.

I believe there should be more discussion around adding people to the
`allowlist`, vs. simply not including as code owners until they become
members, but this change simply formats existing code owners, and
doesn't change anyone's current status.
f7o pushed a commit to f7o/opentelemetry-collector-contrib that referenced this pull request Sep 12, 2024
…ng receiver (open-telemetry#34015)

**Description:** Adding Google Cloud Monitoring receiver

Closes open-telemetry#33762

**Testing:** Test cases for configuration added

**Documentation:** README file added for describing the component
f7o pushed a commit to f7o/opentelemetry-collector-contrib that referenced this pull request Sep 12, 2024
**Description:**
As a result of
open-telemetry#34585,
open-telemetry#34015,
and
open-telemetry#34572,
we've had some inconsistencies and incorrect information introduced
around code owners. This corrects these errors by updating
`metadata.yaml` to match the `CODEOWNERS` file, as well as updating
`CODEOWNERS` format, and adding a user to the `allowlist` of code owners
that are not OpenTelemetry members.

I believe there should be more discussion around adding people to the
`allowlist`, vs. simply not including as code owners until they become
members, but this change simply formats existing code owners, and
doesn't change anyone's current status.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants