opentelemetry-collector-contrib icon indicating copy to clipboard operation
opentelemetry-collector-contrib copied to clipboard

[receiver/gitproviderreceiver] rename to githubreceiver

Open adrielp opened this issue 1 year ago â€Ē 8 comments

Description: <Describe what has changed.> Renames gitproviderreceiver to githubreceiver to better align with OpenTelemetry Semantic Conventions v1.27.0+ and allow for tracing and log signals in the future.

adrielp avatar Aug 19 '24 12:08 adrielp

After the rename, the "provider" part in the name seems a bit (more) awkward. (I think it was me who originally came up with this ðŸĪŠ). I'm not sure what other name would be better.

  • VCS receiver?
  • VCS Metrics receiver?
  • VCS Repository receiver?
  • Repository Change Metrics receiver? 😅
  • ...

I had the same thought @andrzej-stencel. Technically it's still whatever a VCS provides, however, I think vcsmetricsreceiver might be a better option since it's focused on metrics only from version control systems.

adrielp avatar Aug 19 '24 13:08 adrielp

@andrzej-stencel - change has been made.

For whoever merges, check-links will fail as it points to main. It will auto resolve once this PR is merged.

adrielp avatar Aug 19 '24 16:08 adrielp

Out of ignorance, is there any potential of this receiver being able to support logs or traces in the future? (If so, we may not want to have metrics in the name.)

That is a good question. I never planned for that. There is a GitHub Action Receiver that I'm getting ready to contribute that will receiver events from GitHub, turn them into traces, and additional receive the workflow logs. In that structure, it's essentially a web hook receiver.

Is there precedence where both scraping & web hook style receiving occur within the same receiver component? -- If so, I could see that tracing component being combined with this one?

adrielp avatar Aug 20 '24 00:08 adrielp

I'm aware that more discussion is going on in other issues and CNCF Slack around direction to go here, but I'm approving as I'm fine either way. We can move this PR forward once there's a resolution there. (If I'm understanding properly).

sounds good, appreciate it @crobert-1 🙇🏞

adrielp avatar Aug 20 '24 22:08 adrielp

Cross posting the outcome from the SIG decision today https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/27460#issuecomment-2302537838

will update this PR to reflect.

adrielp avatar Aug 21 '24 18:08 adrielp

This is now ready for merge. In future iterations I may make some small internal shifts given that we will be allowing for more signals within this singular component. This component has now been renamed to be the githubreceiver and will contain other signals.

adrielp avatar Aug 25 '24 19:08 adrielp

@andrzej-stencel I know you've already approved, but would you be up for reviewing again with this new rename? Just want to make sure we're agreed here. 👍

crobert-1 avatar Aug 26 '24 16:08 crobert-1

@andrzej-stencel I know you've already approved, but would you be up for reviewing again with this new rename? Just want to make sure we're agreed here. 👍

Sure! :rocket:

andrzej-stencel avatar Aug 27 '24 07:08 andrzej-stencel

Thanks for making the rename! One potential change here would be to move the internal/scraper/githubscraper/ directory up a level to just be internal/scraper, since there's no need to qualify which scraper is being used. Just a thought though, not needed as a part of the rename.

I agree! that's the next thing to do outside this PR!

adrielp avatar Aug 28 '24 17:08 adrielp

I think we can merge this once conflicts are resolved

mx-psi avatar Aug 30 '24 10:08 mx-psi

@mx-psi - conflicts resolved. Unrelated and unexpected failure observed in build & test workflows for the redaction processor. Fixed once #34937 is merged. All workflows are good otherwise.

adrielp avatar Aug 30 '24 12:08 adrielp

Looks like this

  [✖] https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/receiver/githubreceiver/internal/scraper/githubscraper/README.md#github-limitations → Status: 404

Is a real error though?

mx-psi avatar Aug 30 '24 13:08 mx-psi

Looks like this


  [✖] https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/receiver/githubreceiver/internal/scraper/githubscraper/README.md#github-limitations → Status: 404

Is a real error though?

@mx-psi it will be resolved once merged into main because this is going to a main ref for the documentation which doesn't currently exist given that this is a component rename

adrielp avatar Aug 30 '24 13:08 adrielp