-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Migrated Google Cloud Spanner receiver to scraper approach. #5868
Migrated Google Cloud Spanner receiver to scraper approach. #5868
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, didn't know about the scraperhelper! Looks good, just a couple of small adjustments and this is ready to go.
receiver/googlecloudspannerreceiver/internal/metadata/metricsbuilder.go
Outdated
Show resolved
Hide resolved
receiver/googlecloudspannerreceiver/internal/metadata/metricsbuilder_test.go
Outdated
Show resolved
Hide resolved
receiver/googlecloudspannerreceiver/internal/metadata/metricsbuilder_test.go
Outdated
Show resolved
Hide resolved
receiver/googlecloudspannerreceiver/internal/metadata/metricsbuilder_test.go
Show resolved
Hide resolved
receiver/googlecloudspannerreceiver/internal/metadata/metricsbuilder_test.go
Outdated
Show resolved
Hide resolved
receiver/googlecloudspannerreceiver/internal/metadata/metricsdatapoint.go
Outdated
Show resolved
Hide resolved
8584ce8
to
07860c2
Compare
07860c2
to
2b32a66
Compare
@jpkrohling , I resolved all your comments. One job failed, but it is not related to my changes (( |
Created #5874 to address the flaky test. Tests restarted. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. For further PRs, please add new commits instead of squashing your work, as it makes subsequent reviews easier. We can then squash the commits when everything is ready, at merge time.
Ok |
@jpkrohling this time failed unit tests job with unrelated test. Can you trigger re-run for it? |
Issue opened (#5875) and tests restarted. |
@bogdandrutu , @tigrannajaryan , all approvals received. |
Intent of this MR is to migrate receiver implementation to scraper approach - this also will automatically add obsreporting.
These changes were requested by @bogdandrutu in scope of #5831 MR.
FYI: @tigrannajaryan , @jpkrohling