Skip to content

Conversation

cody-littley
Copy link
Contributor

Why are these changes needed?

Adds metrics to the relay server.

Checks

  • I've made sure the tests are passing. Note that there might be a few flaky tests, in that case, please comment that they are not relevant.
  • I've checked the new test coverage and the coverage percentage didn't drop.
  • Testing Strategy
    • Unit tests
    • Integration tests
    • This PR is not tested :(

Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
@cody-littley cody-littley self-assigned this Nov 27, 2024
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
@cody-littley cody-littley mentioned this pull request Dec 3, 2024
5 tasks
Copy link
Contributor

@ian-shim ian-shim left a comment

Choose a reason for hiding this comment

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

lgtm, just few comments

@@ -47,6 +48,9 @@ type cacheAccessor[K comparable, V any] struct {
// cache is the underlying cache that this wrapper manages.
cache Cache[K, V]

// weightCalculator is the function used to calculate the weight of a key-value pair.
calculator WeightCalculator[K, V]
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this calculator or the weight used for?
I feel like abstractions like this make the code bit harder to read

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's used to calculate the size of each individual item in the cache. Things like blobs are straight forward, you just take the length of the byte array. Chunks are much less straight forward, since they are stored in a more complex data structure.

I agree that these types of abstractions add complexity. I'm just not sure how to make the cache sufficiently generic as to work with both blobs and chunks without it.

Signed-off-by: Cody Littley <[email protected]>
relay/metrics.go Outdated
}

// NewRelayMetrics creates a new RelayMetrics instance, which encapsulates all metrics related to the relay.
func NewRelayMetrics(logger logging.Logger, port int) (*RelayMetrics, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks the metric wrapper doesn't make it much concise, especially considering the amount of code added

Signed-off-by: Cody Littley <[email protected]>
@cody-littley cody-littley merged commit 9f68f74 into Layr-Labs:master Dec 6, 2024
6 checks passed
@cody-littley cody-littley deleted the relay-metrics branch December 6, 2024 16:40
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