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] Better defaults for HTTP exporter mode #27007

Merged

Conversation

jmsnll
Copy link
Contributor

@jmsnll jmsnll commented Sep 19, 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.

@jmsnll jmsnll requested a review from a team September 19, 2023 18:18
@github-actions github-actions bot added the cmd/telemetrygen telemetrygen command label Sep 19, 2023
@jmsnll jmsnll force-pushed the fix/default-http-exporter-behaviour branch from b2bc3e0 to b8c2fe9 Compare September 19, 2023 18:28
James Neill added 7 commits September 19, 2023 19:31
…trics and traces

- the default value for each differs therefore it is not a common flag
- returns the default http endpoint whenever http mode is enabled and no endpoint was provided (i.e. its still the default grpc endpoint)
- if the endpoint has been set or we're not in http mode return the underlying value
- whenever no custom endpoint is provided telemetrygen then defaults to either the gRPC or HTTP exporter endpoint
@jmsnll jmsnll force-pushed the fix/default-http-exporter-behaviour branch from b8c2fe9 to c77b37e Compare September 19, 2023 18:31
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @jmsnll, could the path be added for logs as well?

cmd/telemetrygen/internal/metrics/config.go Show resolved Hide resolved
@@ -23,6 +23,9 @@ type Config struct {
// Flags registers config flags.
func (c *Config) Flags(fs *pflag.FlagSet) {
c.CommonFlags(fs)

fs.StringVar(&c.HTTPPath, "otlp-http-url-path", "/v1/traces", "Which URL path to write to")
Copy link
Contributor

Choose a reason for hiding this comment

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

likewise, would be great to have a test for this as well

@jmsnll
Copy link
Contributor Author

jmsnll commented Sep 19, 2023

And thanks for the review @codeboten!

I could be wrong here, but Isn't the URL path only relevant for HTTP? I can add it to the the logs command but since it's only using gRPC at the moment I don't see where the value would be actually used.

@codeboten
Copy link
Contributor

I could be wrong here, but Isn't the URL path only relevant for HTTP? I can add it to the the logs command but since it's only using gRPC at the moment I don't see where the value would be actually used.

Of course... i forgot that logs aren't supported for http yet 🤦🏻 ignore my comment 👍🏻

@jmsnll jmsnll force-pushed the fix/default-http-exporter-behaviour branch from 612b55f to d3bdb2d Compare September 20, 2023 16:22
@github-actions
Copy link
Contributor

github-actions bot commented Oct 5, 2023

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Oct 5, 2023
@mx-psi mx-psi removed the Stale label Oct 5, 2023
@mx-psi mx-psi requested a review from codeboten October 5, 2023 08:08
@codeboten codeboten added the ready to merge Code review completed; ready to merge by maintainers label Oct 5, 2023
@evan-bradley
Copy link
Contributor

@jmsnll Could you fix the linting errors?

@codeboten
Copy link
Contributor

@evan-bradley this is my bad... tried to resolve a conflict via the github UI... will fix

@mx-psi mx-psi merged commit 3f5d64c into open-telemetry:main Oct 6, 2023
82 checks passed
@github-actions github-actions bot added this to the next release milestone Oct 6, 2023
jmsnll added a commit to jmsnll/opentelemetry-collector-contrib that referenced this pull request 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmd/telemetrygen telemetrygen command ready to merge Code review completed; ready to merge by maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants