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

Added implementation of Google Cloud Spanner receiver - 4th part(actual receiver implementation). #5727

Conversation

ydrozhdzhal
Copy link

This part of implementation is the final one and contains actual receiver implementation.
It includes reading and parsing of metadata configuration, initialization of stats readers(project, database) and collection of metrics.

@ydrozhdzhal
Copy link
Author

@jpkrohling , tigrannajaryan, this is the final part of implementation.

@tigrannajaryan
Copy link
Member

@khospodarysko please review as a codeowner.

@ydrozhdzhal ydrozhdzhal force-pushed the googlecloudspannerreceiver/implementation-part-4 branch from 9c43ea0 to b8b4ee1 Compare October 13, 2021 21:01
@ydrozhdzhal ydrozhdzhal force-pushed the googlecloudspannerreceiver/implementation-part-4 branch from b8b4ee1 to f65dc8f Compare October 14, 2021 05:29
receiver/googlecloudspannerreceiver/receiver.go Outdated Show resolved Hide resolved
receiver/googlecloudspannerreceiver/receiver.go Outdated Show resolved Hide resolved
receiver/googlecloudspannerreceiver/receiver.go Outdated Show resolved Hide resolved
receiver/googlecloudspannerreceiver/receiver.go Outdated Show resolved Hide resolved
receiver/googlecloudspannerreceiver/receiver.go Outdated Show resolved Hide resolved
receiver/googlecloudspannerreceiver/receiver.go Outdated Show resolved Hide resolved
receiver/googlecloudspannerreceiver/receiver.go Outdated Show resolved Hide resolved
receiver/googlecloudspannerreceiver/receiver_test.go Outdated Show resolved Hide resolved
@ydrozhdzhal ydrozhdzhal force-pushed the googlecloudspannerreceiver/implementation-part-4 branch 2 times, most recently from 86abddf to efdcd05 Compare October 14, 2021 14:12
@ydrozhdzhal ydrozhdzhal force-pushed the googlecloudspannerreceiver/implementation-part-4 branch from efdcd05 to 3532c74 Compare October 15, 2021 07:42
@ydrozhdzhal
Copy link
Author

@jpkrohling, I resolved all your comments, except one (it is still hasn't been marked as resolved), but reduced time intervals. If you explain your idea by example - I can implement it in scope of this MR, or as a future improvement. Now there is unrelated flaky test failure, issues for it was created a week ago or so(not sure if it is strictly the same, but also related to TestBallastMemory and loadtest job).

@ydrozhdzhal ydrozhdzhal force-pushed the googlecloudspannerreceiver/implementation-part-4 branch 3 times, most recently from 5af9e75 to 0344b01 Compare October 18, 2021 09:36
@jpkrohling jpkrohling added the ready to merge Code review completed; ready to merge by maintainers label Oct 18, 2021
@ydrozhdzhal ydrozhdzhal force-pushed the googlecloudspannerreceiver/implementation-part-4 branch 3 times, most recently from 0744d67 to 6614f99 Compare October 18, 2021 11:11
@jpkrohling
Copy link
Member

Looks like the new test is failing:

go test -race -timeout 300s --tags="" ./...
panic: sync: negative WaitGroup counter

goroutine 122 [running]:
sync.(*WaitGroup).Add(0xc000435714, 0xffffffffffffffff)
	/opt/hostedtoolcache/go/1.17.2/x64/src/sync/waitgroup.go:74 +0x278
sync.(*WaitGroup).Done(...)
	/opt/hostedtoolcache/go/1.17.2/x64/src/sync/waitgroup.go:99
github.com/open-telemetry/opentelemetry-collector-contrib/receiver/googlecloudspannerreceiver.TestStartInitializesDataCollectionWithCollectDataError.func1({0x0, 0x0})
	/home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib/receiver/googlecloudspannerreceiver/receiver_test.go:157 +0x4c
github.com/open-telemetry/opentelemetry-collector-contrib/receiver/googlecloudspannerreceiver.(*googleCloudSpannerReceiver).notifyOnCollectData(0xc0004387e0, {0x0, 0x0})
	/home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib/receiver/googlecloudspannerreceiver/receiver.go:178 +0x96
github.com/open-telemetry/opentelemetry-collector-contrib/receiver/googlecloudspannerreceiver.(*googleCloudSpannerReceiver).Start.func1()
	/home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib/receiver/googlecloudspannerreceiver/receiver.go:81 +0x138
created by github.com/open-telemetry/opentelemetry-collector-contrib/receiver/googlecloudspannerreceiver.(*googleCloudSpannerReceiver).Start
	/home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib/receiver/googlecloudspannerreceiver/receiver.go:73 +0x152
FAIL	github.com/open-telemetry/opentelemetry-collector-contrib/receiver/googlecloudspannerreceiver	0.498s

Perhaps the collectData is being executed more than once, causing the wg to go negative. You can try to wrap that into a sync.Once.

@ydrozhdzhal
Copy link
Author

Looks like the new test is failing:

go test -race -timeout 300s --tags="" ./...
panic: sync: negative WaitGroup counter

goroutine 122 [running]:
sync.(*WaitGroup).Add(0xc000435714, 0xffffffffffffffff)
	/opt/hostedtoolcache/go/1.17.2/x64/src/sync/waitgroup.go:74 +0x278
sync.(*WaitGroup).Done(...)
	/opt/hostedtoolcache/go/1.17.2/x64/src/sync/waitgroup.go:99
github.com/open-telemetry/opentelemetry-collector-contrib/receiver/googlecloudspannerreceiver.TestStartInitializesDataCollectionWithCollectDataError.func1({0x0, 0x0})
	/home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib/receiver/googlecloudspannerreceiver/receiver_test.go:157 +0x4c
github.com/open-telemetry/opentelemetry-collector-contrib/receiver/googlecloudspannerreceiver.(*googleCloudSpannerReceiver).notifyOnCollectData(0xc0004387e0, {0x0, 0x0})
	/home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib/receiver/googlecloudspannerreceiver/receiver.go:178 +0x96
github.com/open-telemetry/opentelemetry-collector-contrib/receiver/googlecloudspannerreceiver.(*googleCloudSpannerReceiver).Start.func1()
	/home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib/receiver/googlecloudspannerreceiver/receiver.go:81 +0x138
created by github.com/open-telemetry/opentelemetry-collector-contrib/receiver/googlecloudspannerreceiver.(*googleCloudSpannerReceiver).Start
	/home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib/receiver/googlecloudspannerreceiver/receiver.go:73 +0x152
FAIL	github.com/open-telemetry/opentelemetry-collector-contrib/receiver/googlecloudspannerreceiver	0.498s

Perhaps the collectData is being executed more than once, causing the wg to go negative. You can try to wrap that into a sync.Once.

Can you point to example or is it something similar to existing code?

@jpkrohling
Copy link
Member

jpkrohling commented Oct 18, 2021

This documentation page should have an example: https://pkg.go.dev/sync#Once . Basically, you'd have the wg.Done() in a sync.Once that is defined outside of the block.

@tigrannajaryan tigrannajaryan removed the ready to merge Code review completed; ready to merge by maintainers label Oct 18, 2021
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 18, 2021

CLA Signed

The committers are authorized under a signed CLA.

  • ✅ ydrozhdzhal (af3f565cc19879e46131dbef4b6fd5da2f12e155)

@asukhyy asukhyy force-pushed the googlecloudspannerreceiver/implementation-part-4 branch 2 times, most recently from af3f565 to 6bcb151 Compare October 18, 2021 13:28
@asukhyy
Copy link

asukhyy commented Oct 18, 2021

@tigrannajaryan - could you please trigger workflows? Thanks.

…al receiver implementation).

This part of implementation is the final one and contains actual receiver implementation.
It includes reading and parsing of metadata configuration, initialization of stats readers(project, database) and collection of metrics.
@ydrozhdzhal ydrozhdzhal force-pushed the googlecloudspannerreceiver/implementation-part-4 branch from 6bcb151 to 193e124 Compare October 18, 2021 13:58
@ydrozhdzhal
Copy link
Author

@tigrannajaryan - could you please trigger workflows? Thanks.

There is no need for this, I recommitted changes.

@ydrozhdzhal
Copy link
Author

@jpkrohling , @tigrannajaryan , it is ready to merge for now (after applying latest fixes).

@tigrannajaryan tigrannajaryan merged commit 253e063 into open-telemetry:main Oct 18, 2021
@ydrozhdzhal ydrozhdzhal deleted the googlecloudspannerreceiver/implementation-part-4 branch October 19, 2021 04:43
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.

5 participants