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

Allow multiple OTLP to be created #2743

Merged
merged 8 commits into from
Mar 22, 2021

Conversation

pjanotti
Copy link
Contributor

Why

To allow the OTLP receiver to be created while one instance is already running. This allows changing the components without restarting the process. See related #2741

What

Description:
Move the registration of the receiver servers to when the receiver is started. Also check in the goroutines serving requests if the errors are not the ones expected when shutting down the receiver (avoid useless calls to ReportFatalError).

Tests:
Added tests to the otlp package to check for the issues being fixed.

@pjanotti pjanotti requested a review from a team March 19, 2021 23:28
@codecov
Copy link

codecov bot commented Mar 19, 2021

Codecov Report

Merging #2743 (b99ed2f) into main (3660f5c) will decrease coverage by 0.02%.
The diff coverage is 75.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2743      +/-   ##
==========================================
- Coverage   91.83%   91.80%   -0.03%     
==========================================
  Files         291      291              
  Lines       15522    15528       +6     
==========================================
+ Hits        14254    14256       +2     
- Misses        867      869       +2     
- Partials      401      403       +2     
Impacted Files Coverage Δ
receiver/otlpreceiver/factory.go 86.66% <ø> (ø)
receiver/otlpreceiver/otlp.go 88.67% <75.00%> (-3.33%) ⬇️

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 3660f5c...b99ed2f. Read the comment docs.

@pjanotti
Copy link
Contributor Author

This test failure shouldn't be related to this change:

--- FAIL: TestJaegerAgentUDP_ThriftCompact (10.32s)
    jaeger_agent_test.go:187: 
        	Error Trace:	jaeger_agent_test.go:187
        	            				jaeger_agent_test.go:52
        	Error:      	Received unexpected error:
        	            	listen udp :50267: bind: An attempt was made to access a socket in a way forbidden by its access permissions.
        	Test:       	TestJaegerAgentUDP_ThriftCompact
        	Messages:   	Start failed
    jaeger_agent_test.go:201: Time out waiting for []
    jaeger_agent_test.go:206: 
        	Error Trace:	jaeger_agent_test.go:206
        	            				jaeger_agent_test.go:52
        	Error:      	Not equal: 
        	            	expected: 1
        	            	actual  : 0
        	Test:       	TestJaegerAgentUDP_ThriftCompact
--- FAIL: TestJaegerAgentUDP_ThriftBinary (10.32s)
    jaeger_agent_test.go:187: 
        	Error Trace:	jaeger_agent_test.go:187
        	            				jaeger_agent_test.go:75
        	Error:      	Received unexpected error:
        	            	listen udp :50271: bind: An attempt was made to access a socket in a way forbidden by its access permissions.
        	Test:       	TestJaegerAgentUDP_ThriftBinary
        	Messages:   	Start failed
    jaeger_agent_test.go:201: Time out waiting for []
    jaeger_agent_test.go:206: 
        	Error Trace:	jaeger_agent_test.go:206
        	            				jaeger_agent_test.go:75
        	Error:      	Not equal: 
        	            	expected: 1
        	            	actual  : 0
        	Test:       	TestJaegerAgentUDP_ThriftBinary
--- FAIL: TestJaegerAgentUDP_ThriftBinary_PortInUse (0.06s)
    jaeger_agent_test.go:92: 
        	Error Trace:	jaeger_agent_test.go:92
        	Error:      	Received unexpected error:
        	            	listen udp :50346: bind: An attempt was made to access a socket in a way forbidden by its access permissions.
        	Test:       	TestJaegerAgentUDP_ThriftBinary_PortInUse
        	Messages:   	Start failed
FAIL

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.

See my comment, the receivers are not safe to be restarted right now, and we need to define these guarantees clearly. Please start by documenting what is the requirement for each component, and think about how can we provide helpers to achieve that.

receiver/otlpreceiver/otlp_test.go Outdated Show resolved Hide resolved
receiver/otlpreceiver/otlp_test.go Outdated Show resolved Hide resolved
@bogdandrutu
Copy link
Member

Is this still needed after #2757? If yes, change it accordingly.

@pjanotti
Copy link
Contributor Author

Is this still needed after #2757? If yes, change it accordingly.

Yes, it is still needed - PR updated.

receiver/otlpreceiver/otlp.go Outdated Show resolved Hide resolved
receiver/otlpreceiver/otlp.go Show resolved Hide resolved
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.

Please resolve the last set of comments

@bogdandrutu bogdandrutu merged commit 705e996 into open-telemetry:main Mar 22, 2021
hughesjj pushed a commit to hughesjj/opentelemetry-collector that referenced this pull request Apr 27, 2023
)

* Bump k8s.io/kubectl from 0.26.2 to 0.26.3 in /tests

Bumps [k8s.io/kubectl](https://github.com/kubernetes/kubectl) from 0.26.2 to 0.26.3.
- [Release notes](https://github.com/kubernetes/kubectl/releases)
- [Commits](kubernetes/kubectl@v0.26.2...v0.26.3)

---
updated-dependencies:
- dependency-name: k8s.io/kubectl
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

* make tidy

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: jeffreyc-splunk <jeffreyc@splunk.com>
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.

2 participants