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.

Sign up for GitHub

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

Jump to bottom

feat: Add framework for BYOID metrics headers #1332

Merged
merged 9 commits into from
Jun 23, 2023
Merged

feat: Add framework for BYOID metrics headers #1332

merged 9 commits into from
Jun 23, 2023

Conversation

aeitzman
Copy link
Contributor

No description provided.

@aeitzman aeitzman requested review from a team as code owners June 12, 2023 16:54
@aeitzman aeitzman changed the title Add framework for BYOID metrics headers feat: Add framework for BYOID metrics headers Jun 12, 2023
@aeitzman aeitzman added the owlbot:run Add this label to trigger the Owlbot post processor. label Jun 12, 2023
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Jun 12, 2023
@arithmetic1728 arithmetic1728 self-requested a review June 12, 2023 19:03
@arithmetic1728 arithmetic1728 added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jun 12, 2023
@arithmetic1728
Copy link
Contributor

Will the byoid metric header go to any of the oauth2.googleapis.com/token, IAM token endpoint, VM metadata server endpoints? Will the header go to chemist, or show up in cloud_apis concord table?

@aeitzman
Copy link
Contributor Author

Will the byoid metric header go to any of the oauth2.googleapis.com/token, IAM token endpoint, VM metadata server endpoints? Will the header go to chemist, or show up in cloud_apis concord table?

No, it is only going to the sts endpoint (and that is all that is planned for the forseeable future) which does not go through chemist or show up in the cloud_apis table.

@arithmetic1728
Copy link
Contributor

Will the byoid metric header go to any of the oauth2.googleapis.com/token, IAM token endpoint, VM metadata server endpoints? Will the header go to chemist, or show up in cloud_apis concord table?

No, it is only going to the sts endpoint (and that is all that is planned for the forseeable future) which does not go through chemist or show up in the cloud_apis table.

Great. Thank you for the confirmation!

@arithmetic1728 arithmetic1728 removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jun 12, 2023
BigTailWolf
BigTailWolf approved these changes Jun 15, 2023
View reviewed changes
Copy link
Contributor

@BigTailWolf BigTailWolf left a comment

Choose a reason for hiding this comment

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

LGTM

@arithmetic1728
Copy link
Contributor

arithmetic1728 commented Jun 15, 2023 •
edited
Loading

please run python -m nox -s blacken to fix the lint errors (and use python -m nox -s lint to verify), also it's complaining line 280 in tests/test_identity_pool.py missed test coverage (you can run python -m nox -s cover to check the coverage after adding more unit tests).

@aeitzman aeitzman added kokoro:run Add this label to force Kokoro to re-run the tests. owlbot:run Add this label to trigger the Owlbot post processor. labels Jun 15, 2023
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Jun 15, 2023
@yoshi-kokoro yoshi-kokoro removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Jun 15, 2023
lsirac
lsirac reviewed Jun 15, 2023
View reviewed changes
google/auth/metrics.py Outdated Show resolved Hide resolved
google/auth/external_account.py Show resolved Hide resolved
google/auth/external_account.py Show resolved Hide resolved
@BigTailWolf BigTailWolf added the owlbot:run Add this label to trigger the Owlbot post processor. label Jun 16, 2023
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Jun 16, 2023
@aeitzman aeitzman added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jun 16, 2023
@aeitzman aeitzman removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jun 23, 2023
@aeitzman aeitzman merged commit 1a8f50c into googleapis:main Jun 23, 2023
@release-please release-please bot mentioned this pull request Jun 23, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clundin25 clundin25 clundin25 left review comments

@arithmetic1728 arithmetic1728 arithmetic1728 approved these changes

@lsirac lsirac lsirac approved these changes

@BigTailWolf BigTailWolf BigTailWolf approved these changes

Assignees

@BigTailWolf BigTailWolf

@lsirac lsirac

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@aeitzman @arithmetic1728 @BigTailWolf @lsirac @clundin25 @yoshi-kokoro