opentelemetry-collector-contrib
opentelemetry-collector-contrib copied to clipboard
open-telemetry
Fix for - syncTargetAllocator in prometheus metrics receiver doesnt detect regex changes in metrics relabel config
Description: <Describe what has changed.> Fixing a bug - With the Targetallocator component in prometheus receiver, when prometheus scrape config is updated for metric relabel config with just regex change, the prometheus metrics receiver doesn't update the hash and hence doesn't pick up the metrics relabel config with the new regex. This is because the regex struct has unexported fields. This fix similar to the fix made in opentelemetry-operator fixes this issue by taking into account the unexported fields as well.
Link to tracking Issue: - #29313
Testing: Tested to make sure that just the regex changes in metric relabeling gets picked up with TargetAllocator enabled.
Reopening this PR since the old one(https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/30258) got closed due to inactivity.
@Aneurysm9 @dashpole - Please take a look and let me know if you have any questions/concerns. Thanks!
@Aneurysm9 @dashpole - Please take a look and let me know if you have any questions/concerns. Thanks!
@Aneurysm9 @dashpole - Gentle ping. Checking back. Pls take a look and let me know if you have any feedback. Thanks!
@Aneurysm9 @dashpole - Please take a look and let me know if you have any questions/concerns. Thanks!
@Aneurysm9 @dashpole - Gentle ping. Checking back. Pls take a look and let me know if you have any feedback. Thanks!
@Aneurysm9 - I have addressed the feedback from the sig meeting. Please take a look. Thanks!
@Aneurysm9 @dashpole - Please take a look and let me know if you have any questions/concerns. Thanks!
@Aneurysm9 @dashpole - Gentle ping. Checking back. Pls take a look and let me know if you have any feedback. Thanks!
@Aneurysm9 - I have addressed the feedback from the sig meeting. Please take a look. Thanks!
@Aneurysm9 - Gentle ping, pls take a look and let me know if you have any feedback. Thanks!
@Aneurysm9 @dashpole - Please take a look and let me know if you have any questions/concerns. Thanks!
@Aneurysm9 @dashpole - Gentle ping. Checking back. Pls take a look and let me know if you have any feedback. Thanks!
@Aneurysm9 - I have addressed the feedback from the sig meeting. Please take a look. Thanks!
@Aneurysm9 - Gentle ping, pls take a look and let me know if you have any feedback. Thanks!
@dashpole / @Aneurysm9 - Could you please take a look at the PR when you get a chance? Thanks!
@Aneurysm9 @dashpole - Please take a look and let me know if you have any questions/concerns. Thanks!
@Aneurysm9 @dashpole - Gentle ping. Checking back. Pls take a look and let me know if you have any feedback. Thanks!
@Aneurysm9 - I have addressed the feedback from the sig meeting. Please take a look. Thanks!
@Aneurysm9 - Gentle ping, pls take a look and let me know if you have any feedback. Thanks!
@dashpole / @Aneurysm9 - Could you please take a look at the PR when you get a chance since its been pending for quite sometime? Thanks!
@rashmichandrashekar could you please rebase your branch, so your PR only has changes to the prometheus receiver? Right now, it has a bunch of other random changes.
@rashmichandrashekar could you please rebase your branch, so your PR only has changes to the prometheus receiver? Right now, it has a bunch of other random changes.
Thanks @swiatekm-sumo. It is done. The changes now are all relevant to the go mod and go sum for the hashstructure module removal.
@djaglowski - Following up. Could you please take a look at this PR? I see that you are assigned to this. Thanks!
@dashpole - Gentle ping. Could you please review when you get a chance? Thanks!
@dashpole - Gentle ping. Could you please review when you get a chance? Thanks!
@dashpole / @Aneurysm9 - Ping. Please let me know if there's any other feedback. We have been waiting on this to be fixed to take the new collector release. Could you also please provide an ETA if possible as to when this might be looked into?
@dashpole - Gentle ping. Could you please review when you get a chance? Thanks!
@dashpole / @Aneurysm9 - Ping. Please let me know if there's any other feedback. We have been waiting on this to be fixed to take the new collector release. Could you also please provide an ETA if possible as to when this might be looked into?
@djaglowski - I see you assigned to this PR. Could you pls take a look and let me know your feedback. Thanks!
I'm not sure I'm willing to accept this. I don't want to own a hashing library. We are working on splitting out the target allocator into its own module in https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/33146 so it can have separate owners. If the future owners of the module want to own a hashing library, thats up to them.
@rashmichandrashekar it looks like we might be moving the target allocator related code to its own package, as per #33146. For that reason, your PR might need to wait until that's decided. Apologies for the wait, and thank you for your patience.
@rashmichandrashekar it looks like we might be moving the target allocator related code to its own package, as per #33146. For that reason, your PR might need to wait until that's decided. Apologies for the wait, and thank you for your patience.
Thanks @swiatekm-sumo for letting me know. Please let me know when the move is done and owners are decided and I can update the PR with any feedback from the owners of the code.
I'm not sure I'm willing to accept this. I don't want to own a hashing library. We are working on splitting out the target allocator into its own module in #33146 so it can have separate owners. If the future owners of the module want to own a hashing library, thats up to them.
Thanks @dashpole. I will follow up with @swiatekm-sumo / future owners to get feedback and update the PR.
@rashmichandrashekar it looks like we might be moving the target allocator related code to its own package, as per #33146. For that reason, your PR might need to wait until that's decided. Apologies for the wait, and thank you for your patience.
Thanks @swiatekm-sumo for letting me know. Please let me know when the move is done and owners are decided and I can update the PR with any feedback from the owners of the code.
@swiatekm-sumo / @dashpole - Are the new owners for the targetallocator component figured out? I can follow up with the right owners while the code is being split to address any comments.
@rashmichandrashekar it looks like we might be moving the target allocator related code to its own package, as per #33146. For that reason, your PR might need to wait until that's decided. Apologies for the wait, and thank you for your patience.
Thanks @swiatekm-sumo for letting me know. Please let me know when the move is done and owners are decided and I can update the PR with any feedback from the owners of the code.
@swiatekm-sumo / @dashpole - Are the new owners for the targetallocator component figured out? I can follow up with the right owners while the code is being split to address any comments.
@swiatekm-sumo - As the new potential code owner, would you like me to update these changes to get rid of the library and have the same changes as the operator repo and address the comment for sorting the keys? Please lmk as this PR has been waiting for a while.
@rashmichandrashekar yeah, in my opinion we should do something very similar to what the target allocator does and just serialize the configs to yaml, and hash the output. They should be sorted, to avoid map ordering inconsistencies.
LGTM
Thanks @swiatekm for the quick review and approval. @dashpole - Could you pls help get this merged?
LGTM
Thanks @swiatekm for the quick review and approval. @dashpole - Could you pls help get this merged?
@dashpole - Following up, could you pls help get this merged as this has been waiting for several months?