-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[service] Validate pipeline type against component types #9257
[service] Validate pipeline type against component types #9257
Conversation
service
: validate pipeline type against component types
service
: validate pipeline type against component types2193534
to
2da328c
Compare
service/service.go
Outdated
@@ -141,6 +146,39 @@ func New(ctx context.Context, set Settings, cfg Config) (*Service, error) { | |||
return srv, nil | |||
} | |||
|
|||
func Validate(ctx context.Context, set Settings, cfg Config) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why public?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it's being called from the otelcol
package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ycombinator please take this as strong feedback that adding new public API to packages is discouraged at this time. Please add a godoc comment and prepare to discuss why this must be public.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, thanks for letting me know about trying not to increase the public API surface. It makes sense. Let me see if I can reimplement this functionality without adding to the public API, otherwise I will add a godoc here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, I'm not really seeing an easy way around adding a new public API here given that the implementation of this function is using symbols from internal
packages. But if there's some way to accomplish not adding
a new public API, I'm open to suggestions on how to go about it.
For now, I've added a godoc comment to this function as suggested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we get the same result by just calling New
? Is there a side effect to calling New
which is being avoided by having a separate function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion, @djaglowski - it totally works! Not sure why I didn't consider that much simpler approach 🤦. Anyway, implemented in e373dd9.
service/service.go
Outdated
// validate the configuration | ||
if err := validate(ctx, set, cfg, srv.telemetrySettings); err != nil { | ||
return nil, err | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would we not find out about this error anyway when we call graph.Build
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's true. I was trying to validate it even earlier than that but you're right — it doesn't really matter. Removing this call, thanks for catching it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed in 8729b39.
service/service.go
Outdated
telSettings := servicetelemetry.TelemetrySettings{ | ||
Logger: tel.Logger(), | ||
TracerProvider: tel.TracerProvider(), | ||
MeterProvider: noop.NewMeterProvider(), | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not using noop
everywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean for the Logger
and TracerProvider
fields? I'm not seeing anything in the noop
package that could be used to initialize those fields.
3218b81
to
8729b39
Compare
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
PR is still active; waiting on review. |
8729b39
to
8eb53a6
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9257 +/- ##
==========================================
- Coverage 91.31% 90.97% -0.34%
==========================================
Files 357 353 -4
Lines 19202 18748 -454
==========================================
- Hits 17534 17056 -478
- Misses 1340 1364 +24
Partials 328 328 ☔ View full report in Codecov by Sentry. |
8b5979d
to
be6c277
Compare
Hi @bogdandrutu, would you mind kicking off the CI checks again on this PR, please? Thanks! |
Thanks @atoulme, for kicking off CI again. I've fixed another linter error (from BTW, I noticed that CI, specifically the
I'm not following how the failing assertion in a test for the |
It's possible contrib CI is broken. Rerunning. |
Thanks. [EDIT] Never mind, I misunderstood what you meant by "contrib CI". I understand now that you were referring to the failing tests, not the fact that you have to manually trigger CI runs for me on this PR. |
Nothing like that, we just run CI by checking out the latest main of contrib, which can break. FWIW CI doesn't run for you on commit because this is your first PR. After you have a commit in the repository, CI will run when you push to your branch without our intervention. |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
Hi @atoulme, could you please guide me on what's needed to merge this PR? I'm afraid it will be closed in 14 days otherwise due to being stale. Thanks! |
you ask for review by pinging approvers. I'm not an approver. You can participate in the SIG meeting as well, which is happening right now, to ask for review. |
972934d
to
382fcae
Compare
@codeboten @bogdandrutu @dmitryax @mx-psi Would one of you have some time to review this PR and merge it if it's ready, please? It's been open for nearly three months now. Thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, merging!
#### Description This PR reverts the change made in open-telemetry#9257 due to problems reported in open-telemetry#10031. <!-- Issue number if applicable --> #### Link to tracking issue Fixes open-telemetry#10031.
Description:
This change adds another layer of validation to pipelines. It validates that all the components in a pipeline are of the same type as the pipeline.
For example, if a
metrics
pipeline contains atraces
-only receiver, theotelcol validate -config ...
command will fail.Link to tracking Issue:
Fixes #8007.
Testing:
Added unit test + existing tests are passing.
Documentation:
godoc.