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

runtime: no longer upping stats for deprecated v2 APIs #19906

Merged
merged 6 commits into from
Feb 17, 2022

Conversation

alyssawilk
Copy link
Contributor

Part of #19847

Signed-off-by: Alyssa Wilk <[email protected]>
Signed-off-by: Alyssa Wilk <[email protected]>
Copy link
Contributor

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

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

Code changes LGTM.

@htuch I think it's ok to remove the deprecated feature usage counter, but would like to have your take on this as well.

htuch
htuch previously requested changes Feb 15, 2022
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Don't we want this as part of minor versioning? I.e. the ability to count features that are in use but deprecated?

@alyssawilk
Copy link
Contributor Author

Minor versioning isn't supported yet right? When it is I think we can work on adding stats which aren't using a to-be-deprecated global variable.

@adisuissa
Copy link
Contributor

Yes I agree that the stats plumbing can be achieved differently.

@htuch
Copy link
Member

htuch commented Feb 15, 2022

I guess what I'm wondering about is whether users should have to change their monitoring systems or we would reuse the same stat name here. If we plan to reuse the stat name, that is fine.

@alyssawilk
Copy link
Contributor Author

either way if the stat isn't relevant now, and the usage relies on illegal singleton access, can we remove and revisit adding back when it's relevant?

@htuch
Copy link
Member

htuch commented Feb 15, 2022

Yes, but please keep in mind reuse of the stat name in teh future.

Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

This is used for current deprecated features, right? Even apart from minor versioning? I think we need to keep this as people are likely alerting/monitoring on this as a way to see configs that need to be updated?

/wait

@alyssawilk
Copy link
Contributor Author

I don't think this is used by deprecated features, if you look at the code it only branches on v2 APIs. AFIK all those docs are out of date as I believe support for feature deprecation was removed when we revved to v3, and has not been added back since.
Adi will be the developer adding it back which is why I gave him first review pass as I figured if he's fine adding it back later, that would be good enough?

Signed-off-by: Alyssa Wilk <[email protected]>
Comment on lines 73 to 75
// The feature is allowed. It is assumed this check is called when the feature
// is about to be used, so increment the feature use stat.
countDeprecatedFeatureUseInternal(stats_);
Copy link
Member

Choose a reason for hiding this comment

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

This ends up getting called from deprecatedFieldHelper and I'm pretty positive will increment today when using a deprecated field which I think means the stats are still in use?

/wait-any

@alyssawilk alyssawilk changed the title runtime: removing v2 stats runtime: no longer upping stats for deprecated v2 APIs Feb 16, 2022
Signed-off-by: Alyssa Wilk <[email protected]>
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks LGTM

@alyssawilk alyssawilk merged commit 8136cce into envoyproxy:main Feb 17, 2022
@alyssawilk alyssawilk deleted the stats branch August 4, 2022 01:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants