Skip to content
This repository has been archived by the owner on Apr 18, 2024. It is now read-only.

Removed unused metrics #154

Merged
merged 2 commits into from
Sep 4, 2023
Merged

Conversation

aarshkshah1992
Copy link
Contributor

@aarshkshah1992 aarshkshah1992 commented Sep 4, 2023

Removes metrics we no longer need as the backpressure PR has landed in main and some of those metrics were put in place to catch old bugs that no longer exist.

@@ -324,92 +307,20 @@ func (p *pool) fetchResource(ctx context.Context, from *Node, resource string, m

// trace-metrics
// request life-cycle metrics

fetchRequestSuccessTimeTraceMetric.WithLabelValues(resourceType, getCacheStatus(isCacheHit), "tcp_connection").Observe(float64(result.TCPConnection.Milliseconds()))
Copy link
Contributor

Choose a reason for hiding this comment

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

do we not want this l1 breakdown timing anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@willscott It hasn't helped us with anything so far. We know where most time is spent on for hits/misses and we already have these same metrics on Saturn as well.
I think the e2e tracing work will be more helpful here.

Copy link
Contributor

Choose a reason for hiding this comment

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

we want to be able to subtract it from overall request latency to separate out the L1 / caboose proportion from the lassie proportion in the long tail, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, okay, no harm in keeping it then. I've put it back.

@aarshkshah1992
Copy link
Contributor Author

@willscott Can I merge ?

@willscott
Copy link
Contributor

waiting for visibility from @AmeanAsad when he wakes up, since it merge-conflicts with his in-progress PR to the same branch

@aarshkshah1992 aarshkshah1992 changed the base branch from feat/adaptive to aa/test-simulator September 4, 2023 08:18
@aarshkshah1992 aarshkshah1992 merged commit a23963d into aa/test-simulator Sep 4, 2023
16 checks passed
@aarshkshah1992 aarshkshah1992 deleted the feat/remove-metrics branch September 4, 2023 08:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants