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/Kafka] feat: Add topic to internal consume-claim metric attributes (35336) #36068

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

StephanSalas
Copy link

@StephanSalas StephanSalas commented Oct 29, 2024

Description

Added the "topic" attribute to the following internal telemetry metrics:

1. KafkaReceiverMessages
2. KafkaReceiverCurrentOffset
3. KafkaReceiverOffsetLag
4. KafkaReceiverUnmarshalFailedSpans
5. KafkaReceiverUnmarshalFailedMetricPoints
6. KafkaReceiverUnmarshalFailedLogRecords

Link to tracking issue

Fixes #35336

Testing Performed

Tested locally and observed new attribute

Documentation Added

No documentation changes needed because internal metric attributes are not typically documented (except for the basic metric details in the existing generated_telemetry.go). Thus they were not added, but let me know if they need to be :)

@StephanSalas StephanSalas requested review from MovieStoreGuy and a team as code owners October 29, 2024 14:15
@github-actions github-actions bot requested a review from pavolloffay October 29, 2024 14:15
@StephanSalas StephanSalas changed the title [Receiver/Kafka] feat: Add topic to internal telemetry attributes (35336) [Receiver/Kafka] feat: Add topic to internal consume-claim metric attributes (35336) Oct 29, 2024
@MovieStoreGuy
Copy link
Contributor

I suggest adding the attributes via the metadata.yml since this will automatically generate the code and the docs for users.

See mdatagen command for reference.

@StephanSalas
Copy link
Author

StephanSalas commented Oct 30, 2024

I suggest adding the attributes via the metadata.yml since this will automatically generate the code and the docs for users.

See mdatagen command for reference.

@MovieStoreGuy, looks like it won't generate docs per my attempts just now and also the bug listed here: open-telemetry/opentelemetry-collector#10926 (comment). The mdatagen attribute schema validates my changes, however it just doesn't generate anything in the docs or otherwise even though both go generate and mdatagen metadata.yaml commands succeed.

Given that, would you prefer I still commit my updated metadata.yaml for this PR, or wait until the mdatagen fix is released (to also check in the gen'd docs)? The latter option may be easier to track as a separate PR for receiver/Kafka and other affected components.

@StephanSalas
Copy link
Author

StephanSalas commented Nov 1, 2024

Hey Reviewers,

Given the issue with the doc generation, can this be merged as-is? I do see a related issue with the metrics that may depend a bit on this as well that I am also working on: #36093.

@StephanSalas
Copy link
Author

Hey Reviewers,

Given the issue with the doc generation, can this be merged as-is? I do see a related issue with the metrics that may depend a bit on this as well that I am also working on: #36093.

@MovieStoreGuy, just wanted to add a bump to this Q. Let me know if that approach is satisfactory while open-telemetry/opentelemetry-collector#10926 (comment) is still an ongoing issue.

@MovieStoreGuy
Copy link
Contributor

I suggest adding the attributes via the metadata.yml since this will automatically generate the code and the docs for users.
See mdatagen command for reference.

@MovieStoreGuy, looks like it won't generate docs per my attempts just now and also the bug listed here: open-telemetry/opentelemetry-collector#10926 (comment). The mdatagen attribute schema validates my changes, however it just doesn't generate anything in the docs or otherwise even though both go generate and mdatagen metadata.yaml commands succeed.

Given that, would you prefer I still commit my updated metadata.yaml for this PR, or wait until the mdatagen fix is released (to also check in the gen'd docs)? The latter option may be easier to track as a separate PR for receiver/Kafka and other affected components.

Annoying but it makes sense.

@StephanSalas
Copy link
Author

I suggest adding the attributes via the metadata.yml since this will automatically generate the code and the docs for users.
See mdatagen command for reference.

@MovieStoreGuy, looks like it won't generate docs per my attempts just now and also the bug listed here: open-telemetry/opentelemetry-collector#10926 (comment). The mdatagen attribute schema validates my changes, however it just doesn't generate anything in the docs or otherwise even though both go generate and mdatagen metadata.yaml commands succeed.
Given that, would you prefer I still commit my updated metadata.yaml for this PR, or wait until the mdatagen fix is released (to also check in the gen'd docs)? The latter option may be easier to track as a separate PR for receiver/Kafka and other affected components.

Annoying but it makes sense.

I have just updated the ChangeLog to hopefully pass the previously failed pipeline.

@StephanSalas
Copy link
Author

QQ as a first time PR contributor. Do the pipelines usually take this long or is it just a quick "hit the reboot button" type of thing?

Stephan

@StephanSalas
Copy link
Author

@MovieStoreGuy, anything we still need to do to get this merged?

Copy link
Contributor

github-actions bot commented Dec 8, 2024

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@MovieStoreGuy MovieStoreGuy added the ready to merge Code review completed; ready to merge by maintainers label Dec 12, 2024
@andrzej-stencel
Copy link
Member

Looks like tests need to be updated?

@andrzej-stencel andrzej-stencel removed the ready to merge Code review completed; ready to merge by maintainers label Dec 16, 2024
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.

Kafka Receiver does not include topic or receiver as a label
4 participants