[cmd/telemetrygen] Better defaults for HTTP exporter mode #27007
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jmsnll
force-pushed
the
fix/default-http-exporter-behaviour
branch
from
b2bc3e0 to
b8c2fe9
Compare
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
…atically included
jmsnll
force-pushed
the
fix/default-http-exporter-behaviour
branch
from
b8c2fe9 to
c77b37e
Compare
codeboten
reviewed
Sep 19, 2023
| @@ -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") | |||
There was a problem hiding this comment.
likewise, would be great to have a test for this as well
|
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 |
Of course... i forgot that logs aren't supported for http yet 🤦🏻 ignore my comment 👍🏻 |
jmsnll
force-pushed
the
fix/default-http-exporter-behaviour
branch
from
612b55f to
d3bdb2d
Compare
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
mx-psi
approved these changes
Oct 5, 2023
codeboten
approved these changes
Oct 5, 2023
codeboten
added
the
ready to merge
|
@jmsnll Could you fix the linting errors? |
|
@evan-bradley this is my bad... tried to resolve a conflict via the github UI... will fix |
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 <aboten@lightstep.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description:
otlp-http-url-pathflag from the common flags for each commandtracescommand with the default/v1/tracesmetricscommand with the default/v1/metricslogscommand so no longer exposed--otlp-endpointnow defaults to empty string and targets a new attributeCustomEndpointoncommon.ConfigEndpoint()getter function to thecommon.Configstruct for retrieving the correct endpointCustomEndpointis empty then eitherDefaultGRPCEndpointorDefaultHTTPEndpointare chosen based upon the value ofconfig.UseHTTPexporters.goin both modulesLink 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-insecurenow correctly sends traces using HTTP to the default HTTP endpoint.Documentation:
No documentation added/changed but updated some command flag descriptions.