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

Fix nil panic from otlp exporter #6633

Merged
merged 3 commits into from
Nov 29, 2022
Merged

Fix nil panic from otlp exporter #6633

merged 3 commits into from
Nov 29, 2022

Conversation

pavankrish123
Copy link
Contributor

@pavankrish123 pavankrish123 commented Nov 29, 2022

Description:

Fixing a bug - The OTLP exporter panics when collector tries to shut down the pipelines as soon as an error happens during start up.

An error is startup of the exporter will leave exporter with nil connection, followed by shutdown method of exporter invokes calling close on nil connection leading to panic

In the earlier versions since shutdown methods were never called when Start fails, we never observed this behavior.

Link to tracking Issue: #6619

Testing: Manual testing to see collector shuts down gracefully without any panic

2022/11/28 21:50:31 proto: duplicate proto type registered: jaeger.api_v2.PostSpansRequest
2022/11/28 21:50:31 proto: duplicate proto type registered: jaeger.api_v2.PostSpansResponse
2022-11-28T21:50:31.796-0800	info	service/telemetry.go:111	Setting up own telemetry...
2022-11-28T21:50:31.796-0800	info	service/telemetry.go:141	Serving Prometheus metrics	{"address": ":8888", "level": "Basic"}
2022-11-28T21:50:31.796-0800	debug	components/components.go:28	Stable component.	{"kind": "exporter", "data_type": "traces", "name": "otlp/auth", "stability": "Stable"}
2022-11-28T21:50:31.796-0800	debug	components/components.go:28	Stable component.	{"kind": "receiver", "name": "otlp", "pipeline": "traces", "stability": "Stable"}
2022-11-28T21:50:31.796-0800	info	service/service.go:89	Starting otelcontribcol...	{"Version": "v0.42.0-2740-g7f4d4425a0", "NumCPU": 12}
2022-11-28T21:50:31.797-0800	info	extensions/extensions.go:41	Starting extensions...
2022-11-28T21:50:31.797-0800	info	extensions/extensions.go:44	Extension is starting...	{"kind": "extension", "name": "basicauth"}
2022-11-28T21:50:31.797-0800	info	extensions/extensions.go:48	Extension started.	{"kind": "extension", "name": "basicauth"}
2022-11-28T21:50:31.797-0800	info	pipelines/pipelines.go:74	Starting exporters...
2022-11-28T21:50:31.797-0800	info	pipelines/pipelines.go:78	Exporter is starting...	{"kind": "exporter", "data_type": "traces", "name": "otlp/auth"}
2022-11-28T21:50:31.797-0800	info	zapgrpc/zapgrpc.go:174	[core] [Channel #1] Channel created	{"grpc_log": true}
2022-11-28T21:50:31.797-0800	info	zapgrpc/zapgrpc.go:174	[core] [Channel #1] Channel Connectivity change to SHUTDOWN	{"grpc_log": true}
2022-11-28T21:50:31.797-0800	info	zapgrpc/zapgrpc.go:174	[core] [Channel #1] Channel deleted	{"grpc_log": true}
error
<nil>
2022-11-28T21:50:31.797-0800	info	service/service.go:115	Starting shutdown...
2022-11-28T21:50:31.797-0800	info	pipelines/pipelines.go:118	Stopping receivers...
2022-11-28T21:50:31.797-0800	info	pipelines/pipelines.go:125	Stopping processors...
2022-11-28T21:50:31.797-0800	info	pipelines/pipelines.go:132	Stopping exporters...
2022-11-28T21:50:31.797-0800	info	extensions/extensions.go:55	Stopping extensions...
2022-11-28T21:50:31.797-0800	info	service/service.go:129	Shutdown complete.
Error: cannot start pipelines: grpc: the credentials require transport level security (use grpc.WithTransportCredentials() to set)
2022-11-28 21:50:31.797531 I | collector server run finished with error: cannot start pipelines: grpc: the credentials require transport level security (use grpc.WithTransportCredentials() to set)

@pavankrish123 pavankrish123 requested review from a team and Aneurysm9 November 29, 2022 05:55
@codecov
Copy link

codecov bot commented Nov 29, 2022

Codecov Report

Base: 91.21% // Head: 91.21% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (9ed665a) compared to base (ae077be).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6633   +/-   ##
=======================================
  Coverage   91.21%   91.21%           
=======================================
  Files         245      245           
  Lines       14194    14197    +3     
=======================================
+ Hits        12947    12950    +3     
  Misses        997      997           
  Partials      250      250           
Impacted Files Coverage Δ
exporter/otlpexporter/otlp.go 93.40% <100.00%> (+0.22%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@bogdandrutu
Copy link
Member

I am fine with the fix, not sure I would call this a bug or enhancement since anyway the collector failed to start. Please add a .chloggen entry

@bogdandrutu
Copy link
Member

You still need to add the chloggen entry https://github.com/open-telemetry/opentelemetry-collector/blob/main/Makefile#L484

@pavankrish123
Copy link
Contributor Author

@bogdandrutu please let me know if we are good changelog wise.

@bogdandrutu bogdandrutu merged commit 67866ce into open-telemetry:main Nov 29, 2022
@pavankrish123 pavankrish123 deleted the authpanic branch November 29, 2022 17:30
jaronoff97 pushed a commit to lightstep/opentelemetry-collector that referenced this pull request Dec 14, 2022
* fix nil panic from otlp exporter.

* fix nil panic from otlp exporter.

* fix nil panic from otlp exporter.
jaronoff97 pushed a commit to lightstep/opentelemetry-collector that referenced this pull request Dec 14, 2022
* fix nil panic from otlp exporter.

* fix nil panic from otlp exporter.

* fix nil panic from otlp exporter.
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