-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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/prometheusremotewrite] Add component skeleton #35295
base: main
Are you sure you want to change the base?
Conversation
.github/CODEOWNERS
Outdated
@@ -271,6 +271,7 @@ receiver/otlpjsonfilereceiver/ @open-teleme | |||
receiver/podmanreceiver/ @open-telemetry/collector-contrib-approvers @rogercoll | |||
receiver/postgresqlreceiver/ @open-telemetry/collector-contrib-approvers @djaglowski | |||
receiver/prometheusreceiver/ @open-telemetry/collector-contrib-approvers @Aneurysm9 @dashpole | |||
receiver/prometheusremotewritereceiver/ @open-telemetry/collector-contrib-approvers @ArthurSens |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should add the sponsor as a codeowner: https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/CONTRIBUTING.md#adding-new-components
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andrzej-stencel, I've added you here since you were the sponsor a few months ago. I'm not sure if you're still willing to be the sponsor though!
Let me know if you're not and I can look for another :)
Yeah, I noticed all this wrong stuff in the owners but I'm struggling to understand how the automation works here. I was hoping to see CI failures and then a useful error message that shows me how to fix it :) |
Try running the |
279bd63
to
6839df6
Compare
I just did, it solved a few problems but apparently didn't re-generate the README file. Let me see what CI tells me now |
6839df6
to
fd68009
Compare
fd68009
to
477d6f6
Compare
Signed-off-by: Arthur Silva Sens <[email protected]>
477d6f6
to
38dbe7c
Compare
) | ||
|
||
// Config holds common fields and embedded protocol-specific configurations | ||
type Config struct{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the first PR include the component's specific configuration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hoped to keep this PR as small as possible just to get things started, I haven't tried implementing anything yet so I'm not 100% certain how the final config will look. If that's a requirement I can start implementing the whole thing, but sounds like a bit too much for an initial PR 😅
But anyways, I'm the newcomer here. I trust what you reviewers decide as the best first step :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see! TBH I'm not sure if that's a strong requirement. I'm fine either way if there is no other objection!
Description:
Add skeleton for Prometheus Remote-Write Receiver. With the release of PrometheusRemoteWritev2, there's commitment from the Prometheus team to implement and maintain this component :)
prometheus/prometheus#12633 (comment)
Link to tracking Issue: #33782