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

fix: reduce time bucket size #1143

Merged
merged 2 commits into from
Feb 25, 2023
Merged

fix: reduce time bucket size #1143

merged 2 commits into from
Feb 25, 2023

Conversation

birdychang
Copy link
Contributor

No description provided.

@@ -203,7 +203,7 @@ var DefaultViews = []*view.View{
{
Measure: PersistDuration,
Aggregation: defaultMillisecondsDistribution,
TagKeys: []tag.Key{TaskType, Table, ActorCode},
TagKeys: []tag.Key{TaskType, Table},
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, why is ActorCode removed?

Copy link
Member

Choose a reason for hiding this comment

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

High cardinality, maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I check the metric and code, ActorCode was never included. So I'm removing it from TagKeys.

Copy link
Member

Choose a reason for hiding this comment

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

Ohh its probably because we forget to include it when logging the metric...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. I think the cardinality is already high enough :P

@@ -9,7 +9,7 @@ import (
"go.opencensus.io/tag"
)

var defaultMillisecondsDistribution = view.Distribution(0.01, 0.05, 0.1, 0.3, 0.6, 0.8, 1, 2, 3, 4, 5, 6, 8, 10, 13, 16, 20, 25, 30, 40, 50, 65, 80, 100, 130, 160, 200, 250, 300, 400, 500, 650, 800, 1000, 2000, 5000, 10000, 20000, 30000, 50000, 100000, 200000, 500000, 1000000, 2000000, 5000000, 10000000, 10000000)
Copy link
Member

Choose a reason for hiding this comment

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

@birdychang I assume this change is the primary motivation of the PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

Comment on lines 154 to 155
stop := metrics.Timer(ctx, metrics.StateExtractionDuration)
defer stop()
// stop := metrics.Timer(ctx, metrics.StateExtractionDuration)
// defer stop()
Copy link
Member

Choose a reason for hiding this comment

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

What is the motivation for this the removal of this? If we do mean to remove it, let's opt to delete the lines instead of commenting it out. And if this is the only location this metric is instrumented, let's delete the metric too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we figure out a better way is to drop unwanted metrics in prometheus. I'll revert this.

@birdychang birdychang changed the title fix: disable StateExtractionDuration and reduce time bucket size fix: reduce time bucket size Feb 24, 2023
@birdychang birdychang merged commit 5ece2fa into master Feb 25, 2023
@birdychang birdychang deleted the fix_metrics branch February 25, 2023 01:13
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.

3 participants