Feat: expand kafka broker metrics

[jcountsNR]

Rebasing and resubmitting from the original PR due to new team ownership.

ORIGINAL PR: 23646

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: jcountsNR / name: Joseph Counts (9b7332f, 1212594, 21b3de0, f671fec, fdd6ac6, aa373ce, ffcda1d, 277a38d, 863cfd6, 12b6be5, 7f3dfa4, 44a7d7d, bac4529, b1f7f4f, 69fa596, 9b5ce27, a85f2c3, 3d24044, f5ab9a2, 075d149, 7d92fea, 25d0e71, 06048d4, 4a3a149, 2616287, 46cc504, 0c09dd1, 12f7942, 172e172, 2b12c34, 0702096, 42c74cd, e137f3d, 380b044, a255ec1, 1cf6c15, 65862db, ad34e0b, a894856, 3391a70, b5b8bc1, ec75617, dafb75d, e8a0cb5, 051fa90, 1a4993b, 5085e98, 1fb7d9b, afea5c1, 1009d64, c5c8c3b, 4b80265, 3fc9412, 4298096, 2368562, 38ed735)

[jcountsNR]

@dmitryax , I didn't have a way to update the other PR since I did not raise it, so unfortunately I had to create a new one. I did however rebase with main and run make gotidy.

For the question that was posed in the previous PR -
'Isn't count the cumulative sum?' - New Relic views count as a tally of number of metrics rather than a sum.
For example, if 10 metrics are sent each with a value of 10, the count would be 10, and the sum would be 100. Hopefully that clarifies.

[dmitryax]

'Isn't count the cumulative sum?' - New Relic views count as a tally of number of metrics rather than a sum.
For example, if 10 metrics are sent each with a value of 10, the count would be 10, and the sum would be 100. Hopefully that clarifies.

Ok, but this is not a New Relic specific component. Sums are usually preferable in OpenTelemetry because they are more resilient to data points misses. You can use https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/processor/cumulativetodeltaprocessor to translate all the cumulative metrics to deltas.

Also, please take a look at metrics already defined in the Semantic Conventions https://github.com/open-telemetry/semantic-conventions/blob/ca881074f6df330bacdaa89372f2851c26545fee/docs/messaging/kafka.md?plain=1#L104C13-L104C29. messaging.kafka.network.io is already defined as a sum (cumulative counter) and the definition seems like the same as kafka.brokers.incoming_byte_rate and kafka.brokers.outgoing_byte_rate. We should probably suggest changing it to messaging.kafka.brokers.network.io.

[jcountsNR]

'Isn't count the cumulative sum?' - New Relic views count as a tally of number of metrics rather than a sum.
For example, if 10 metrics are sent each with a value of 10, the count would be 10, and the sum would be 100. Hopefully that clarifies.

Ok, but this is not a New Relic specific component. Sums are usually preferable in OpenTelemetry because they are more resilient to data points misses. You can use https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/processor/cumulativetodeltaprocessor to translate all the cumulative metrics to deltas.

Also, please take a look at metrics already defined in the Semantic Conventions https://github.com/open-telemetry/semantic-conventions/blob/ca881074f6df330bacdaa89372f2851c26545fee/docs/messaging/kafka.md?plain=1#L104C13-L104C29. messaging.kafka.network.io is already defined as a sum (cumulative counter) and the definition seems like the same as kafka.brokers.incoming_byte_rate and kafka.brokers.outgoing_byte_rate. We should probably suggest changing it to messaging.kafka.brokers.network.io.

Got it. I'm just catching up on this integration, but I'm thinking that count does work as a sum, and we might have misunderstood that originally.

For the second point, changing the name to messaging.kafka.brokers.network.io makes total sense to me. Will this track both incoming and outgoing byte rate though? Because we had 2 unique counters defined here.

[dmitryax]

Will this track both incoming and outgoing byte rate though? Because we had 2 unique counters defined here.

Yes, the metric currently defined in the spec messaging.kafka.network.io has state attribute with in and out values

[dmitryax]

There is a PR to semantic conventions originally submitted with this change open-telemetry/semantic-conventions#113. I added a couple of comments, and hopefully we will get more attention from approvers/maintainers of that project

[jcountsNR]

understood, I may have to resubmit that one as well because I don't have privileges on that fork. I see your comments though, I'll make those changes and hopefully make that easy for approvers.

Just so I'm clear, what is the order of approval on both of these? Do I need to get that one approved prior to this one or the other way around?

[dmitryax]

Just so I'm clear, what is the order of approval on both of these? Do I need to get that one approved prior to this one or the other way around?

Ideally, yes. Otherwise, metrics submitted here are more likely to be changed later. But feel free to work on both in parallel. This PR has some CI failures that should be fixed. We also need to start adding them to the new messaging. namespace even if other metrics are not migrated yet.

[jcountsNR]

@dmitryax I had the same issue with other PR, I didn't have access to make changes to that fork so I had to create a new one. I did review your suggestions and I made the requested changes though, so hopefully this new one is in a more ready state.

open-telemetry/semantic-conventions#196

[jcountsNR]

@dmitryax commenting back on this one. I saw the request to run make generate in the failure logs, but in my local it's giving me an error that the executable file is not found in $PATH (mac). I'm not sure what file needs to be in the PATH so I'm having a hard time adding it.

[dmitryax]

@dmitryax commenting back on this one. I saw the request to run make generate in the failure logs, but in my local it's giving me an error that the executable file is not found in $PATH (mac). I'm not sure what file needs to be in the PATH so I'm having a hard time adding it.

You don't have make installed? xcode-select --install should install all the dev tools

[jcountsNR]

@dmitryax commenting back on this one. I saw the request to run make generate in the failure logs, but in my local it's giving me an error that the executable file is not found in $PATH (mac). I'm not sure what file needs to be in the PATH so I'm having a hard time adding it.

You don't have make installed? xcode-select --install should install all the dev tools

I do have it installed. It starts to run but it gives an error on mdatagen executable.

doc.go:5: running "mdatagen": exec: "mdatagen": executable file not found in $PATH

[dmitryax]

Please try make install-tools first

[jcountsNR]

@jcountsNR can you please add the broker metrics to the integration test?

Sorry for the delay on this one. I could not get the test to run locally, so I had to run it in EC2, hence all the commits. I added the broker metrics to the integration test, and it passes when I run it, and I verified that the metrics are being collected as well by running the collector with everything enabled. The only thing I'm missing is writing to the expected.yaml, for some reason it keeps writing a blank object, but it is working so I don't know why it's doing that.

[jcountsNR]

@dmitryax , let me know if this works. I added the metrics to the test, and I also tested the collector to make sure they were coming.

[dmitryax]

Please remove this. This line is intender to be executed once locally to produce the expected.yaml file

[dmitryax]

We have all the metrics enabled, but the expected.yaml output doesn't have anything new. It means the integration test doesn't work some reason

[jcountsNR]

Hmm, I never got it to write anything to expected.yaml, but I was getting metrics from it when I ran the collector. That does seem to indicate it's an issue in the integration test, but the only thing I've changed here is just enabling the metrics.

[github-actions]

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

[github-actions]

Closed as inactive. Feel free to reopen if this PR is still being worked on.