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

Create a dynamodb collector #190

Merged
merged 6 commits into from
Jan 22, 2024
Merged

Conversation

dmanc
Copy link
Contributor

@dmanc dmanc commented Jan 18, 2024

Why are these changes needed?

https://prometheus.io/docs/instrumenting/writing_exporters/#collectors

Allows us to add a metric for the number of blobs in the processing state.

image

Checks

  • I've made sure the lint is passing in this PR.
  • 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.
  • Testing Strategy
    • Unit tests
    • Integration tests
    • This PR is not tested :(

@dmanc dmanc changed the title Create a dynamodb exporter on batcher Create a dynamodb collector on batcher Jan 19, 2024
@dmanc dmanc requested review from ian-shim and jianoaix January 19, 2024 00:50
@dmanc dmanc marked this pull request as ready for review January 19, 2024 00:51
Copy link
Contributor

@jianoaix jianoaix left a comment

Choose a reason for hiding this comment

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

lg

disperser/batcher/metrics.go Outdated Show resolved Hide resolved
disperser/batcher/metrics.go Outdated Show resolved Hide resolved
disperser/batcher/metrics.go Outdated Show resolved Hide resolved
@dmanc dmanc changed the title Create a dynamodb collector on batcher Create a dynamodb collector Jan 20, 2024
@dmanc dmanc requested review from jianoaix and ian-shim January 20, 2024 02:38
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.
Left few comments re: testing. Also, is the screenshot in the PR description with the latest update or with the previous version?

common/aws/dynamodb/client.go Outdated Show resolved Hide resolved
common/aws/dynamodb/client.go Show resolved Hide resolved
@dmanc
Copy link
Contributor Author

dmanc commented Jan 22, 2024

LGTM. Left few comments re: testing. Also, is the screenshot in the PR description with the latest update or with the previous version?

Yeah I'll update the image.

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, left some comments, feel free to address them now or later

common/aws/dynamodb/client_test.go Show resolved Hide resolved
}

func (collector *DynamoDBCollector) Collect(ch chan<- prometheus.Metric) {
count, err := collector.blobMetadataStore.GetBlobMetadataByStatusCount(context.Background(), disperser.Processing)
Copy link
Contributor

Choose a reason for hiding this comment

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

how do we make this track other statuses?

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 would add another value to the channel

ch <- prometheus.MustNewConstMetric(
		collector.blobStatusMetric,
		prometheus.GaugeValue,
		float64(count), <- Different count
		disperser.Processing.String(), <- Change to other status
	)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For this PR, gonna keep it scoped to just processing status. If we want to measure the other ones I'll create a PR.

@dmanc dmanc merged commit 630fe33 into Layr-Labs:master Jan 22, 2024
4 checks passed
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