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/mongodb] add receiver.mongodb.removeDatabaseAttr Alpha feature gate #27621

Merged

Conversation

sakulali
Copy link
Contributor

@sakulali sakulali commented Oct 11, 2023

Description:
Add receiver.mongodb.removeDatabaseAttr Alpha feature gate to remove duplicate database name attribute.

Link to tracking Issue:
#24972

Testing:
make generate
make chlog-validate
go test for mongodbreceiver

Manually tested, built image and manually enabled and disabled this feature gate.

This is the warning we publish:

2023-10-15 17:06:29 2023-10-15T09:06:29.358Z warn [email protected]/scraper.go:62 Feature gate receiver.mongodb.removeDatabaseAttr is not enabled. Please see the README for more information: https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/receiver/mongodbreceiver/README.md {"kind": "receiver", "name": "mongodb", "data_type": "metrics"}

Documentation:

@sakulali sakulali requested a review from djaglowski as a code owner October 11, 2023 13:11
@sakulali sakulali requested a review from a team October 11, 2023 13:11
@github-actions github-actions bot requested a review from schmikei October 11, 2023 13:11
@sakulali sakulali force-pushed the fix-mongodbreceiver-attributes branch from 94bca89 to f5279be Compare October 11, 2023 14:06
Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but can we do this with a feature gate?

@sakulali
Copy link
Contributor Author

LGTM, but can we do this with a feature gate?

Thanks for the reviews @djaglowski! I will look into how to implement it using feature gates.

@djaglowski
Copy link
Member

Thanks @sakulali

@sakulali sakulali force-pushed the fix-mongodbreceiver-attributes branch 2 times, most recently from 4d40348 to f0c1b64 Compare October 15, 2023 09:29
@sakulali sakulali changed the title [receiver/mongodb] remove duplicate database name attribute [receiver/mongodb] add receiver.mongodb.removeDatabaseResourceAttr Alpha feature gate Oct 15, 2023
@sakulali sakulali changed the title [receiver/mongodb] add receiver.mongodb.removeDatabaseResourceAttr Alpha feature gate [receiver/mongodb] add receiver.mongodb.removeDatabaseResourceAttr Alpha feature gate Oct 15, 2023
@sakulali sakulali force-pushed the fix-mongodbreceiver-attributes branch from f0c1b64 to d8414e9 Compare October 15, 2023 09:45
@sakulali
Copy link
Contributor Author

sakulali commented Oct 16, 2023

Hello @djaglowski , i would greatly appreciate it if you could kindly review the code changes of the feature gate.

@sakulali sakulali requested a review from djaglowski October 17, 2023 03:53
@djaglowski
Copy link
Member

@sakulali, I think we should leave database as a resource attribute and remove it as a datapoint attribute. Apologies, this should have been stated in the issue and/or noticed in the initial review.

A couple reasons here:

  1. Databases are noteworthy components which can be described individually. This works in favor of it each being a separate resource.
  2. The problem with changing resource attributes is that it requires reorganizing the data as well. For example, without the database noted as a resource attributes, we would emit multiple resources with the same set of resource attributes. (See some of the testdata files.)

@sakulali
Copy link
Contributor Author

I think we should leave database as a resource attribute and remove it as a datapoint attribute. Apologies, this should have been stated in the issue and/or noticed in the initial review.

Thanks for thoughtful reply @djaglowski. The reasons you mentioned make sense, and I will take some time to make the necessary adjustments to the code.

@sakulali sakulali force-pushed the fix-mongodbreceiver-attributes branch from d8414e9 to c7df02e Compare October 23, 2023 14:50
@sakulali sakulali changed the title [receiver/mongodb] add receiver.mongodb.removeDatabaseResourceAttr Alpha feature gate [receiver/mongodb] add receiver.mongodb.removeDatabaseAttr Alpha feature gate Oct 23, 2023
@sakulali sakulali force-pushed the fix-mongodbreceiver-attributes branch 4 times, most recently from c7802ed to 25662ba Compare October 24, 2023 16:35
@sakulali
Copy link
Contributor Author

I think we should leave database as a resource attribute and remove it as a datapoint attribute

Hello @djaglowski, sorry fo delay reply, i tried to remove datapoint attribute with a feature gate and replace with resource attribute, it seems that regardless of whether the feature gate is enabled or not, the database attribute will be removed. Could you mind help reviews again?

@djaglowski
Copy link
Member

Thanks for continuing on with this @sakulali.

it seems that regardless of whether the feature gate is enabled or not, the database attribute will be removed.

Can you explain why and what the purpose of the feature gate would be in this case?

@sakulali
Copy link
Contributor Author

Can you explain why and what the purpose of the feature gate would be in this case?

Thanks for your patient guidance @djaglowski :). I don't have a thorough understanding of feature gates. From my understanding, the purpose of a feature gate is to inform users that there will be changes to telemetry in future versions, giving downstream users sufficient time to make adjustments. In transitional versions, users can enable the feature gate to adopt new functionality or disable it to retain the original functionality.

@djaglowski
Copy link
Member

@sakulali, that's my understanding as well, but what doesn't make sense to me is why we would include a feature gate if it isn't going to actually control anything. That my interpretation of what you said it seems that regardless of whether the feature gate is enabled or not, the database attribute will be removed, but perhaps I misunderstood.

To be clear, I think we need the feature gate, but it should control whether or not the the database attribute is present.

@sakulali
Copy link
Contributor Author

To be clear, I think we need the feature gate, but it should control whether or not the the database attribute is present.

@djaglowski, thanks very much for helping me correct my mistake and clarifying the proper approach and significance of the feature gate. I will take some more time to think about how to implement this feature gate.

@sakulali sakulali marked this pull request as draft October 31, 2023 06:25
@sakulali sakulali changed the title [receiver/mongodb] add receiver.mongodb.removeDatabaseAttr Alpha feature gate WIP: [receiver/mongodb] add receiver.mongodb.removeDatabaseAttr Alpha feature gate Oct 31, 2023
Copy link
Contributor

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

@github-actions github-actions bot added the Stale label Nov 15, 2023
@sakulali sakulali force-pushed the fix-mongodbreceiver-attributes branch 4 times, most recently from e967d37 to 2fa4409 Compare November 21, 2023 10:41
@sakulali sakulali force-pushed the fix-mongodbreceiver-attributes branch from 2fa4409 to 39c4dad Compare November 21, 2023 11:11
@sakulali
Copy link
Contributor Author

To be clear, I think we need the feature gate, but it should control whether or not the the database attribute is present.

Sorry for delay. Thanks for your attentive correction, could you help to reviews again @djaglowski? During this time, i have been contemplating how to implement this feature. I came across PR 29152 by chance and found some inspiration from it. I would like to express my gratitude to @mx-psi for that.

@sakulali sakulali marked this pull request as ready for review November 21, 2023 14:43
@sakulali sakulali changed the title WIP: [receiver/mongodb] add receiver.mongodb.removeDatabaseAttr Alpha feature gate [receiver/mongodb] add receiver.mongodb.removeDatabaseAttr Alpha feature gate Nov 21, 2023
@github-actions github-actions bot removed the Stale label Nov 22, 2023
Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for persisting through this effort @sakulali.

@djaglowski djaglowski merged commit c27d056 into open-telemetry:main Nov 27, 2023
85 checks passed
@github-actions github-actions bot added this to the next release milestone Nov 27, 2023
@sakulali sakulali deleted the fix-mongodbreceiver-attributes branch November 27, 2023 22:46
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.

4 participants