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

[otlpexporter] Validate endpoint has port #9505

Closed
atoulme opened this issue Feb 7, 2024 · 7 comments
Closed

[otlpexporter] Validate endpoint has port #9505

atoulme opened this issue Feb 7, 2024 · 7 comments
Assignees
@Kimbohlovette
Labels
exporter/otlp good first issue Good for newcomers

Comments

@atoulme
Copy link
Contributor

atoulme commented Feb 7, 2024 

2024-02-07T02:18:55.694Z	error	exporterhelper/queue_sender.go:123	Exporting failed. No more retries left. Dropping data.	{"kind": "exporter", "data_type": "traces", "name": "otlp/XXX", "error": "rpc error: code = Unavailable desc = connection error: desc = \"transport: Error while dialing: dial tcp: address example.com: missing port in address\"", "dropped_items": 335}
go.opentelemetry.io/collector/exporter/exporterhelper.(*queueSender).consume
	go.opentelemetry.io/collector/exporter@v0.91.0/exporterhelper/queue_sender.go:123
go.opentelemetry.io/collector/exporter/exporterhelper/internal.(*boundedMemoryQueue[...]).Consume
	go.opentelemetry.io/collector/exporter@v0.91.0/exporterhelper/internal/bounded_memory_queue.go:55
go.opentelemetry.io/collector/exporter/exporterhelper/internal.(*QueueConsumers[...]).Start.func1
	go.opentelemetry.io/collector/exporter@v0.91.0/exporterhelper/internal/consumers.go:43

If you omit the port from the endpoint used by the otlp exporter, it starts ok but fails to run with an error. Instead, we should have a default port or validate the endpoint on start.
@atmask
Copy link
Contributor

atmask commented Feb 22, 2024

@atoulme I'm just getting familiar with the project and looking for an entry point to get started with. Could I take a look into this if there's not a time constraint (might have to do a bit of environment set up)? Is there a preference between setting the default port or doing the validation?

@atmask
Copy link
Contributor

atmask commented Feb 23, 2024

Okay I've found how the otlp exporter makes use of the configgrpc.Config in the exporter config. I think that the solution I will add is to also validate that a port is specified as part of the otlp exporter's config validation.

This was referenced Feb 23, 2024
Merged
Merged
Closed
TylerHelmuth pushed a commit to open-telemetry/opentelemetry-collector-contrib that referenced this issue Feb 28, 2024
@atmask
[loadbalancingexporter] Set the default otlp port 4317 in factory and…
cea1de2 
… unit test (#31425)

**Description:** 
As described in the otel-collector [issue
9505](open-telemetry/opentelemetry-collector#9505),
the otlpexporter does not function correctly if no port is defined. To
resolve this, the otlp config validation has been updated to fail fast
when the endpoint within an otlp config does not have a port specified.

The loadbalancingexporter config has the otlp exporter config as a
dependency, however, the loadbalancing exporter does not define a port
on the otlpexporter config in two places:
- default config from factory
- testdata contents

This is currently a blocker to the contrib tests for the
[PR](open-telemetry/opentelemetry-collector#9632)
to resolve issue 9505

Relates to:
open-telemetry/opentelemetry-collector#9523

#31371

#31381


**Link to tracking Issue:** 
otel-collector-contrib: [issue
31426](#31426)
Arises from otel-collector [issue
9505](open-telemetry/opentelemetry-collector#9505)

**Testing:** Used `replace` to test loadbalancingexporter changes pass
tests successfully when using the otlpexporter changes from
[PR](open-telemetry/opentelemetry-collector#9632)
XinRanZhAWS pushed a commit to XinRanZhAWS/opentelemetry-collector-contrib that referenced this issue Mar 13, 2024
@atmask @XinRanZhAWS
[loadbalancingexporter] Set the default otlp port 4317 in factory and…
8aa6bec 
… unit test (open-telemetry#31425)

**Description:** 
As described in the otel-collector [issue
9505](open-telemetry/opentelemetry-collector#9505),
the otlpexporter does not function correctly if no port is defined. To
resolve this, the otlp config validation has been updated to fail fast
when the endpoint within an otlp config does not have a port specified.

The loadbalancingexporter config has the otlp exporter config as a
dependency, however, the loadbalancing exporter does not define a port
on the otlpexporter config in two places:
- default config from factory
- testdata contents

This is currently a blocker to the contrib tests for the
[PR](open-telemetry/opentelemetry-collector#9632)
to resolve issue 9505

Relates to:
open-telemetry/opentelemetry-collector#9523

open-telemetry#31371

open-telemetry#31381


**Link to tracking Issue:** 
otel-collector-contrib: [issue
31426](open-telemetry#31426)
Arises from otel-collector [issue
9505](open-telemetry/opentelemetry-collector#9505)

**Testing:** Used `replace` to test loadbalancingexporter changes pass
tests successfully when using the otlpexporter changes from
[PR](open-telemetry/opentelemetry-collector#9632)
@Kimbohlovette
Copy link
Contributor

Hello @Aneurysm9 can I take this issue
I would like to work on this issue

@Kimbohlovette Kimbohlovette mentioned this issue Mar 14, 2024
Closed
2 tasks
@atmask
Copy link
Contributor

atmask commented Mar 14, 2024 •
edited
Loading

@Kimbohlovette There is already an approved PR to resolve this issue. Please see: #9632. No work remaining here.

@Kimbohlovette
Copy link
Contributor

Alright @atmask thanks very much
I will look for another issue

@atmask
Copy link
Contributor

atmask commented Mar 26, 2024 •
edited
Loading

@atoulme This has been stalled in PR review for a month with two approvals but is missing code owner approval. Can we please get a review from a code owner?

dmitryax pushed a commit that referenced this issue Mar 27, 2024
@atmask
[otlp exporter] Validate exporter endpoint has port (#9632)
8dd42ec 
**Description:** This PR updates the otlp exporter config validation to
ensure that the "endpoint" specified for the exporter includes a port.
The goal of this is to fail fast if the configuration is invalid instead
of waiting for an error to arise. The PR adds a function to the
ClientConfig defined in configgrpc that parses the port defined in the
endpoint. The otlp exporter uses this port parsing to validate that

**Link to tracking Issue:** [Issue
9505](#9505)
@atoulme atoulme mentioned this issue Apr 2, 2024
Merged
@atoulme
Copy link
Contributor Author

atoulme commented Apr 12, 2024

Closing as complete.

@atoulme atoulme closed this as completed Apr 12, 2024
@chrkl chrkl mentioned this issue May 22, 2024
Closed
@codeboten codeboten mentioned this issue Jun 21, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Kimbohlovette Kimbohlovette

Labels
exporter/otlp good first issue Good for newcomers
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@atoulme @atmask @Kimbohlovette @andrzej-stencel