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

GoogleCloudSpannerReceiver: Mask lock stats PII #16343

Merged
merged 6 commits into from
Nov 22, 2022

Conversation

architjugran
Copy link
Contributor

@architjugran architjugran commented Nov 17, 2022

Description: Cloud Spanner receiver should have the capability to mask sensitive information. This PR adds a configurable option to mask the PII in lock stats metrics for customers.

NOTE THAT THIS PR WAS ALREADY BEING REVIEWED AT #14607 BUT GOT CLOSED BECAUSE INACTIVE

For the metric "top minute lock stats", the label "row_range_start_key" (https://cloud.google.com/spanner/docs/introspection/lock-statistics#explain-row-range) has PII information of table key values. We have now given the ability to hash the keys individually such that
table_name(key1,key2) becomes table_name(hash1,hash2).
Note that even though key values are hashed, a user can identify if sets of keys share common prefix like table1(key1,key2) and table1(key1,key3)

Link to tracking Issue: #16349

Testing: Unit tests added. Manual end-to-end testing done by running the service locally and verifying the hashed row_range_start_key values.

Documentation: Added the configuration option and the corresponding info in the README

changelog

.

Code review changes
@architjugran architjugran requested review from a team and Aneurysm9 November 17, 2022 10:20
@architjugran
Copy link
Contributor Author

@mx-psi Request you to review. You had earlier reviewed this at #14607 but it got closed because of inactivity. Apologies for the inconvenience.

Thank you so much for your time,
Archit

@dashpole
Copy link
Contributor

Link to tracking Issue: Google-Internal customer-request.

Even though you are addressing a customer request to Google, it is still good to open an issue describing what you are attempting to do. Leave out customer details, but the collector maintainers should be able to understand why you need the functionality. It is also a good place to list what you've tried, but doesn't meet your use-case (e.g. redaction or transform processor).

@architjugran
Copy link
Contributor Author

architjugran commented Nov 17, 2022

Link to tracking Issue: Google-Internal customer-request.

Even though you are addressing a customer request to Google, it is still good to open an issue describing what you are attempting to do. Leave out customer details, but the collector maintainers should be able to understand why you need the functionality. It is also a good place to list what you've tried, but doesn't meet your use-case (e.g. redaction or transform processor).

Done. Opened issue at #16349 and linked to it in this PR's description!

@dashpole PTAL at the updated PR again. Thank you!

Copy link
Contributor

@dashpole dashpole left a comment

Choose a reason for hiding this comment

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

lgtm overall

@dashpole
Copy link
Contributor

Can you ask one of the other CODEOWNERS (@ydrozhdzhal @asukhyy @khospodarysko) to review as well?

@architjugran
Copy link
Contributor Author

architjugran commented Nov 17, 2022

Can you ask one of the other CODEOWNERS (@ydrozhdzhal @asukhyy @khospodarysko) to review as well?

Unfortunately they no longer work with us on this code as the contract has ended.

@ydrozhdzhal @asukhyy would it be possible to have a look at the PR? Thanks!

@dashpole
Copy link
Contributor

If they are no longer active as approvers, we should remove them from CODEOWNERS. I can approve this PR in that case. Are you able to nominate anyone else who would have enough context to review PRs to this component? A single active approver isn't ideal.

@architjugran
Copy link
Contributor Author

architjugran commented Nov 18, 2022

If they are no longer active as approvers, we should remove them from CODEOWNERS. I can approve this PR in that case. Are you able to nominate anyone else who would have enough context to review PRs to this component? A single active approver isn't ideal.

Yes, I will add Varun and Kiranmayi from Google, to be a codeowner in the next PR

Update : Done at #16385

@architjugran
Copy link
Contributor Author

@Aneurysm9 gentle reminder for review. Thank you!

@architjugran
Copy link
Contributor Author

@varunraiko is now added as a code owner and can review instead of @Aneurysm9

@bogdandrutu bogdandrutu merged commit c7cb633 into open-telemetry:main Nov 22, 2022
shalper2 pushed a commit to shalper2/opentelemetry-collector-contrib that referenced this pull request Dec 6, 2022
* Hide lock stats PII

changelog

.

Code review changes

* chlog

* correct merge conflicts

* remove extra space

* code review changes

* Added another test case
@plantfansam plantfansam mentioned this pull request Jul 21, 2023
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.

3 participants