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

[cmd/telemetrygen] Bad default argument values for HTTP exporter mode #26999

Closed
jmsnll opened this issue Sep 19, 2023 · 6 comments
Closed

[cmd/telemetrygen] Bad default argument values for HTTP exporter mode #26999

jmsnll opened this issue Sep 19, 2023 · 6 comments
Assignees
Labels
bug Something isn't working cmd/telemetrygen telemetrygen command priority:p1 High Stale

Comments

@jmsnll
Copy link
Contributor

jmsnll commented Sep 19, 2023

Component(s)

cmd/telemetrygen

What happened?

Description

The default behaviour of telemetrygen when trying to use HTTP exporters is broken out of the box.

  1. otlp-http-url-path is current set in internal/common/config.go file with a default value of /v1/metrics so it is pointing toward the wrong endpoint when trying to use the traces sub-command
  2. otlp-http is used to indicate weather to use gRPC or HTTP exporters but otlp-endpoint always has a default value pointing toward the gRPC receiver port 4317

Steps to Reproduce

Have a locally running collector with HTTP and gRPC receivers enabled, then run telemetrygen to send traces using HTTP with: telemetrygen traces --otlp-http --otlp-insecure

Expected Result

  1. Because we are sending traces otlp-http-url-path has a value of /v1/traces by default
  2. Because we have set the otlp-http flag and have not set a value for otlp-endpoint we send traces to localhost:4318
$ telemetrygen traces --otlp-http --otlp-insecure 
INFO    traces/traces.go:59     starting HTTP exporter
INFO    traces/traces.go:120    generation of traces isn't being throttled
INFO    traces/worker.go:92     traces generated        {"worker": 0, "traces": 1}
INFO    traces/traces.go:80     stop the batch span processor
INFO    traces/traces.go:70     stopping the exporter

Actual Result

We attempt to send traces via HTTP to the wrong endpoint (/v1/metrics) and the incorrect default port for the HTTP receiver :4317

$ telemetrygen traces --otlp-http --otlp-insecure
INFO    traces/traces.go:59     starting HTTP exporter
INFO    traces/traces.go:120    generation of traces isn't being throttled
INFO    traces/worker.go:92     traces generated        {"worker": 0, "traces": 1}
INFO    traces/traces.go:80     stop the batch span processor
traces export: Post "http://localhost:4317/v1/metrics": net/http: HTTP/1.x transport connection broken: malformed HTTP response "\x00\x00\x06\x04\x00\x00\x00\x00\x00\x00\x05\x00\x00@\x00"
INFO    traces/traces.go:70     stopping the exporter

Proposed Solution

  • Remove the declaration of otlp-http-url-path from the common flags and have one in each of the internal/traces/config.go and internal/metrics/config.go module with the appropriate default value.
  • Check if otlp-http has been set and override otlp-endpoint to localhost:4318 if it has it's default value of localhost:4317

Collector version

v0.85.0

Environment information

No response

OpenTelemetry Collector configuration

No response

Log output

No response

Additional context

No response

@jmsnll jmsnll added bug Something isn't working needs triage New item requiring triage labels Sep 19, 2023
@jmsnll jmsnll changed the title [cmd/telemetrygen] Bad default argument values for HTTP exporter [cmd/telemetrygen] Bad default argument values for HTTP exporter mode Sep 19, 2023
@github-actions github-actions bot added the cmd/telemetrygen telemetrygen command label Sep 19, 2023
@github-actions
Copy link
Contributor

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@mx-psi mx-psi added priority:p1 High and removed needs triage New item requiring triage labels Sep 19, 2023
@mx-psi
Copy link
Member

mx-psi commented Sep 19, 2023

Thanks for the report @jmsnll, assigning P1 since this is important to fix for alignment with the OTLP spec

@jmsnll
Copy link
Contributor Author

jmsnll commented Sep 19, 2023

My pleasure! I'm happy to open a PR for this also if my proposed solution looks sound

@mx-psi
Copy link
Member

mx-psi commented Sep 19, 2023

My pleasure! I'm happy to open a PR for this also if my proposed solution looks sound

The proposed solution makes sense to me, feel free to open a PR! :)

mx-psi pushed a commit that referenced this issue Oct 6, 2023
**Description:**

- Refactored and moved `otlp-http-url-path` flag from the common flags
for each command
    - Added flag to the `traces` command with the default `/v1/traces`
    - Added flag to the `metrics` command with the default `/v1/metrics`
    - Flag was not used for the `logs` command so no longer exposed
- Handle changing the default endpoint based on the communication mode
(gRPC or HTTP)
- Flag `--otlp-endpoint` now defaults to empty string and targets a new
attribute `CustomEndpoint` on `common.Config`
- Added the `Endpoint()` getter function to the `common.Config` struct
for retrieving the correct endpoint
- When `CustomEndpoint` is empty then either `DefaultGRPCEndpoint` or
`DefaultHTTPEndpoint` are chosen based upon the value of
`config.UseHTTP`
- **Misc**: Split out the creation of trace/metric exporters into
standalone factory functions with docstrings available in `exporters.go`
in both modules
- **Misc**: Removed the "default value is" from the descriptions of some
flags as it was repeated information

**Link to tracking Issue:** #26999

**Testing:** 

Added new set of unit tests to cover the addition of the
`config.Endpoint()` getter.

Running `telemetrygen traces --otlp-http --otlp-insecure` now correctly
sends traces using HTTP to the default HTTP endpoint.

**Documentation:** 

No documentation added/changed but updated some command flag
descriptions.

---------

Co-authored-by: Alex Boten <[email protected]>
jmsnll added a commit to jmsnll/opentelemetry-collector-contrib that referenced this issue Nov 12, 2023
…etry#27007)

**Description:**

- Refactored and moved `otlp-http-url-path` flag from the common flags
for each command
    - Added flag to the `traces` command with the default `/v1/traces`
    - Added flag to the `metrics` command with the default `/v1/metrics`
    - Flag was not used for the `logs` command so no longer exposed
- Handle changing the default endpoint based on the communication mode
(gRPC or HTTP)
- Flag `--otlp-endpoint` now defaults to empty string and targets a new
attribute `CustomEndpoint` on `common.Config`
- Added the `Endpoint()` getter function to the `common.Config` struct
for retrieving the correct endpoint
- When `CustomEndpoint` is empty then either `DefaultGRPCEndpoint` or
`DefaultHTTPEndpoint` are chosen based upon the value of
`config.UseHTTP`
- **Misc**: Split out the creation of trace/metric exporters into
standalone factory functions with docstrings available in `exporters.go`
in both modules
- **Misc**: Removed the "default value is" from the descriptions of some
flags as it was repeated information

**Link to tracking Issue:** open-telemetry#26999

**Testing:** 

Added new set of unit tests to cover the addition of the
`config.Endpoint()` getter.

Running `telemetrygen traces --otlp-http --otlp-insecure` now correctly
sends traces using HTTP to the default HTTP endpoint.

**Documentation:** 

No documentation added/changed but updated some command flag
descriptions.

---------

Co-authored-by: Alex Boten <[email protected]>
Copy link
Contributor

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@github-actions github-actions bot added the Stale label Nov 20, 2023
@mx-psi
Copy link
Member

mx-psi commented Nov 20, 2023

Fixed by #27007

@mx-psi mx-psi closed this as completed Nov 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cmd/telemetrygen telemetrygen command priority:p1 High Stale
Projects
None yet
Development

No branches or pull requests

2 participants