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.

Sign up for GitHub

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

Jump to bottom

Remove goroutine spawning in favor of the async nature of UDP #794

Merged
merged 1 commit into from
Aug 25, 2020
Merged

Remove goroutine spawning in favor of the async nature of UDP #794

merged 1 commit into from
Aug 25, 2020

Conversation

sonofachamp
Copy link
Contributor

@sonofachamp sonofachamp commented Aug 24, 2020 •
edited
Loading

Follow up from feedback in #566

Description:

Remove goroutine spawn in favor of the async nature of UDP.

Link to tracking Issue:
#566 (comment)

Testing:
Existing local unit/integ tests and manually.

Documentation:

@sonofachamp sonofachamp requested a review from a team August 24, 2020 16:40
bogdandrutu
bogdandrutu reviewed Aug 24, 2020
View reviewed changes
Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

Why go.sum updates? Can you run make gotidy in a separate PR?

@codecov
Copy link

codecov bot commented Aug 24, 2020 •
edited
Loading

Codecov Report

Merging #794 into master will decrease coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #794      +/-   ##
==========================================
- Coverage   87.87%   87.83%   -0.04%     
==========================================
  Files         221      221              
  Lines       12019    12013       -6     
==========================================
- Hits        10562    10552      -10     
- Misses       1103     1106       +3     
- Partials      354      355       +1
Flag Coverage Δ
#integration 69.45% <ø> (ø)
#unit 87.65% <100.00%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
receiver/statsdreceiver/transport/udp_server.go 80.39% <100.00%> (-2.07%) ⬇️
...eiver/awsxrayreceiver/internal/udppoller/poller.go 97.56% <0.00%> (-2.44%) ⬇️
receiver/carbonreceiver/transport/tcp_server.go 65.71% <0.00%> (-1.91%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d33fb59...95542e5. Read the comment docs.

@sonofachamp
Copy link
Contributor Author

@bogdandrutu should I run that from root, or the receiver/statsdreceiver directory? make gotidy seems to partially fail and reports a ton of changes (maybe due to the partial failure).

→ make gotidy 
make for-all CMD="rm -fr go.sum"
make[1]: Entering directory 'opentelemetry-collector-contrib'
running rm -fr go.sum in ./exporter/alibabacloudlogserviceexporter
running rm -fr go.sum in ./exporter/awsxrayexporter
running rm -fr go.sum in ./exporter/azuremonitorexporter
running rm -fr go.sum in ./exporter/carbonexporter
running rm -fr go.sum in ./exporter/elasticexporter
running rm -fr go.sum in ./exporter/honeycombexporter
running rm -fr go.sum in ./exporter/jaegerthrifthttpexporter
running rm -fr go.sum in ./exporter/kinesisexporter
running rm -fr go.sum in ./exporter/lightstepexporter
running rm -fr go.sum in ./exporter/newrelicexporter
running rm -fr go.sum in ./exporter/sapmexporter
running rm -fr go.sum in ./exporter/sentryexporter
running rm -fr go.sum in ./exporter/signalfxexporter
running rm -fr go.sum in ./exporter/splunkhecexporter
running rm -fr go.sum in ./exporter/stackdriverexporter
running rm -fr go.sum in ./extension/observer
running rm -fr go.sum in ./extension/observer/hostobserver
running rm -fr go.sum in ./extension/observer/k8sobserver
running rm -fr go.sum in ./internal/common
running rm -fr go.sum in ./internal/common/awsxray/testdata/sampleapp
running rm -fr go.sum in ./internal/common/awsxray/testdata/sampleserver
running rm -fr go.sum in ./processor/k8sprocessor
running rm -fr go.sum in ./processor/metricstransformprocessor
running rm -fr go.sum in ./processor/resourcedetectionprocessor
running rm -fr go.sum in ./receiver/awsecscontainermetricsreceiver
running rm -fr go.sum in ./receiver/awsxrayreceiver
running rm -fr go.sum in ./receiver/carbonreceiver
running rm -fr go.sum in ./receiver/collectdreceiver
running rm -fr go.sum in ./receiver/k8sclusterreceiver
running rm -fr go.sum in ./receiver/kubeletstatsreceiver
running rm -fr go.sum in ./receiver/prometheusexecreceiver
running rm -fr go.sum in ./receiver/receivercreator
running rm -fr go.sum in ./receiver/redisreceiver
running rm -fr go.sum in ./receiver/sapmreceiver
running rm -fr go.sum in ./receiver/signalfxreceiver
running rm -fr go.sum in ./receiver/simpleprometheusreceiver
running rm -fr go.sum in ./receiver/statsdreceiver
running rm -fr go.sum in ./receiver/wavefrontreceiver
running rm -fr go.sum in ./testbed
make[1]: Leaving directory 'opentelemetry-collector-contrib'
make for-all CMD="go mod tidy"
make[1]: Entering directory 'opentelemetry-collector-contrib'
running go mod tidy in ./exporter/alibabacloudlogserviceexporter
go: downloading golang.org/x/tools v0.0.0-20200724022722-7017fd6b1305
running go mod tidy in ./exporter/awsxrayexporter
go: downloading github.com/Azure/go-autorest/autorest/mocks v0.3.0
go: downloading github.com/googleapis/gnostic v0.4.1
go: finding module for package github.com/googleapis/gnostic/OpenAPIv2
go: downloading github.com/googleapis/gnostic v0.5.1
github.com/open-telemetry/opentelemetry-collector-contrib/exporter/awsxrayexporter tested by
	github.com/open-telemetry/opentelemetry-collector-contrib/exporter/awsxrayexporter.test imports
	go.opentelemetry.io/collector/config/configcheck tested by
	go.opentelemetry.io/collector/config/configcheck.test imports
	go.opentelemetry.io/collector/service/defaultcomponents imports
	go.opentelemetry.io/collector/receiver/prometheusreceiver imports
	github.com/prometheus/prometheus/discovery imports
	github.com/prometheus/prometheus/discovery/kubernetes imports
	k8s.io/client-go/kubernetes imports
	k8s.io/client-go/discovery imports
	github.com/googleapis/gnostic/OpenAPIv2: module github.com/googleapis/gnostic@latest found (v0.5.1), but does not contain package github.com/googleapis/gnostic/OpenAPIv2
Makefile:59: recipe for target 'for-all' failed
make[1]: *** [for-all] Error 1
make[1]: Leaving directory 'opentelemetry-collector-contrib'
Makefile:50: recipe for target 'gotidy' failed
make: *** [gotidy] Error 2

git status:

→ git status
On branch master
Your branch is up to date with 'origin/master'.

Changes not staged for commit:
  (use "git add/rm <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

	modified:   exporter/alibabacloudlogserviceexporter/go.sum
	modified:   exporter/awsxrayexporter/go.sum
	deleted:    exporter/azuremonitorexporter/go.sum
	deleted:    exporter/carbonexporter/go.sum
	deleted:    exporter/elasticexporter/go.sum
	deleted:    exporter/honeycombexporter/go.sum
	deleted:    exporter/jaegerthrifthttpexporter/go.sum
	deleted:    exporter/kinesisexporter/go.sum
	deleted:    exporter/lightstepexporter/go.sum
	deleted:    exporter/newrelicexporter/go.sum
	deleted:    exporter/sapmexporter/go.sum
	deleted:    exporter/sentryexporter/go.sum
	deleted:    exporter/signalfxexporter/go.sum
	deleted:    exporter/splunkhecexporter/go.sum
	deleted:    exporter/stackdriverexporter/go.sum
	deleted:    extension/observer/go.sum
	deleted:    extension/observer/hostobserver/go.sum
	deleted:    extension/observer/k8sobserver/go.sum
	modified:   go.sum
	deleted:    internal/common/awsxray/testdata/sampleapp/go.sum
	deleted:    internal/common/awsxray/testdata/sampleserver/go.sum
	deleted:    internal/common/go.sum
	deleted:    processor/k8sprocessor/go.sum
	deleted:    processor/metricstransformprocessor/go.sum
	deleted:    processor/resourcedetectionprocessor/go.sum
	deleted:    receiver/awsecscontainermetricsreceiver/go.sum
	deleted:    receiver/awsxrayreceiver/go.sum
	deleted:    receiver/carbonreceiver/go.sum
	deleted:    receiver/collectdreceiver/go.sum
	deleted:    receiver/k8sclusterreceiver/go.sum
	deleted:    receiver/kubeletstatsreceiver/go.sum
	deleted:    receiver/prometheusexecreceiver/go.sum
	deleted:    receiver/receivercreator/go.sum
	deleted:    receiver/redisreceiver/go.sum
	deleted:    receiver/sapmreceiver/go.sum
	deleted:    receiver/signalfxreceiver/go.sum
	deleted:    receiver/simpleprometheusreceiver/go.sum
	deleted:    receiver/statsdreceiver/go.sum
	modified:   receiver/statsdreceiver/testdata/config.yaml
	deleted:    receiver/wavefrontreceiver/go.sum
	deleted:    testbed/go.sum

anuraaga
anuraaga approved these changes Aug 25, 2020
View reviewed changes
Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

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

Change LGTM - I've never been able to figure out how to keep go.sum changes out of PRs in this repo unfortunately :(

@sonofachamp
Copy link
Contributor Author

@bogdandrutu I removed all the go.sum file changes.

@bogdandrutu bogdandrutu merged commit 339355d into open-telemetry:master Aug 25, 2020
@sonofachamp sonofachamp deleted the remove-concurrency branch August 25, 2020 20:18
codeboten pushed a commit that referenced this pull request Nov 23, 2022
@toumorokoshi
chore: installation of test packages in eachdist (#794)
d672ca2 
eachdist.py did not support the installation of test packages,
(as defined by the extra_requires:test package group). As a
result, test packages were being added to dev-requirements.txt

By having eachdist.py develop install test packages, and moving
develop/test package definitions to the individual instrumentations,
it is easier to determine which packages require which dependencies
for testing purposes, and enables support for existing dependencies
that follow the extra_requires:test pattern.
codeboten pushed a commit that referenced this pull request Nov 23, 2022
@ocelotl
Use __name__ instead of __file__ (#794)
5993329 
Fixes #787
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bogdandrutu bogdandrutu bogdandrutu approved these changes

@anuraaga anuraaga anuraaga approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sonofachamp @anuraaga @bogdandrutu