[receiver/githubreceiver] add skeleton for tracing GH Actions events #34944
Conversation
|
@msarahan - Thanks for starting this contribution! I appreciate the quick turnaround on getting the skeleton opened up here. A couple things to start off with to get this PR ready for merge:
Sounds right 👍🏼
Agreed with a slight change. until the Webhook has functionality, I don't think this should change. And when it does become functionally available, we should check to ensure at least one of metrics/traces is configured in the event that a user forgets to configure either. |
msarahan
changed the title
|
Thanks for the review, @adrielp. I think I fixed up most of it, except
I found that making this change generated code that assumed that there was a happy path for tracing, but this PR does not add that happy path. Also, there's an error when running your prep.sh script that I've been trying to understand: It seems like the "real" error is |
I see that component lifecycle tests are generated for testing and fail with: │ │
│=== Failed │
│=== FAIL: . TestComponentLifecycle/traces-shutdown (0.00s) │
│ generated_component_test.go:59: │
│ Error Trace: /Users/adrielperkins/dev/trash/opentelemetry-collector-contrib/receiver/githubreceiver/generated_component_test.go:59 │
│ Error: Received unexpected error: │
│ telemetry type is not supported │
│ Test: TestComponentLifecycle/traces-shutdown │
│ │
│=== FAIL: . TestComponentLifecycle/traces-lifecycle (0.00s) │
│ generated_component_test.go:65: │
│ Error Trace: /Users/adrielperkins/dev/trash/opentelemetry-collector-contrib/receiver/githubreceiver/generated_component_test.go:65 │
│ Error: Received unexpected error: │
│ telemetry type is not supported │
│ Test: TestComponentLifecycle/traces-lifecycle │
│ │
│=== FAIL: . TestComponentLifecycle (2.01s) I need to do the in-depth code review beyond the core fundamentals already mentioned. I'm thinking we should be able to add a tiny bit more to make this pass where the receiver with traces configured can run no-op. (the indepth code review is on my todo list)
So looks like this is a bug in golanglint-ci. I went in on your branch and checked out running it against other receivers. I ended up doing the following: cd opentelemetry-collector-contrib/internal/tools
go get -u github.com/golangci/golangci-lint && make tidy
cd - && cd receiver/githubreceiver
../../.tools/golangci-lint --config ../../.golangci.yml runDoing so updated golanglint-ci from |
| "go.opentelemetry.io/collector/confmap" | ||
| "go.opentelemetry.io/collector/otelcol/otelcoltest" | ||
| "go.opentelemetry.io/collector/receiver/scraperhelper" | ||
|
|
||
| "github.com/open-telemetry/opentelemetry-collector-contrib/receiver/githubreceiver/internal" | ||
| "github.com/open-telemetry/opentelemetry-collector-contrib/receiver/githubreceiver/internal/metadata" | ||
| "github.com/open-telemetry/opentelemetry-collector-contrib/receiver/githubreceiver/internal/scraper/githubscraper" | ||
| "github.com/open-telemetry/opentelemetry-collector-contrib/receiver/githubreceiver/internal/webhook" | ||
| "github.com/open-telemetry/opentelemetry-collector-contrib/receiver/webhookeventreceiver" |
There was a problem hiding this comment.
I don't think we should be importing the webhookevent receiver component as part of our tests here and instead should follow the pattern the webhookevent receiver uses to test itself.
There was a problem hiding this comment.
I'm not sure what you mean here. I am using the webhookreceiver to try to avoid reimplementing the webhook functionality. I'm interpreting your request as "please don't use code from other receivers" - which I'm fine to respect. One thing that I've tried to interpret this is to use anonymous structs that have the same fields as the expected webhook configuration struct. Struct stuff doesn't really work that way (much to my Python-headed duck-typing consternation), so the next nearest thing is an interface with getter-type functions. Am I on the right track, or do you mean something else?
One thing I'm a little hung up on is that the Validate() method for the webhookreceiver's config is being called somehow, and I can't figure out when or how. Perhaps this is one reason why you're avoiding using the webhookreceiver.Config struct. I have a debugger breakpoint on the only line where that can be added to our error collection, and the breakpoint is never hit. Nevertheless, the tests fail with:
=== RUN TestLoadConfig
config_test.go:36:
Error Trace: /home/msarahan/code/opentelemetry-collector-contrib/receiver/githubreceiver/config_test.go:36
Error: Received unexpected error:
receivers::github/customname: missing receiver server endpoint from config
Test: TestLoadConfig
--- FAIL: TestLoadConfig (0.00s)
=== RUN TestLoadConfig_Traces
config_test.go:89:
Error Trace: /home/msarahan/code/opentelemetry-collector-contrib/receiver/githubreceiver/config_test.go:89
Error: Received unexpected error:
receivers::github/customname: missing receiver server endpoint from config
Test: TestLoadConfig_Traces
There was a problem hiding this comment.
I am using the webhookreceiver to try to avoid reimplementing the webhook functionality. I'm interpreting your request as "please don't use code from other receivers" - which I'm fine to respect. One thing that I've tried to interpret this is to use anonymous structs that have the same fields as the expected webhook configuration struct.
I think my suggestion is to re-implement it's functionality so that this component is entirely decoupled from any changes to the WebHook receiver component. This removes a dependency that can change separately from this code.
@crobert-1, @TylerHelmuth, @andrzej-stencel - I'm curious to hear your thoughts here and get your opinion.
| @@ -40,6 +43,14 @@ func TestLoadConfig(t *testing.T) { | |||
| defaultConfigGitHubScraper.(*Config).Scrapers = map[string]internal.Config{ | |||
| githubscraper.TypeStr: (&githubscraper.Factory{}).CreateDefaultConfig(), | |||
| } | |||
| defaultConfigGitHubScraper.(*Config).WebhookConfig = webhook.Config{ | |||
There was a problem hiding this comment.
Webhook should be separate from defaultConfigGitHubScraper.
There was a problem hiding this comment.
I renamed this defaultConfigGithubReceiver to make it more clear that it is the top-level configuration struct. I also renamed WebhookConfig to Tracing.
Does this resolve your concern, or is it still needing further alteration?
msarahan
force-pushed
the
githubreceiver-actions-traces
branch
from
134a656 to
6e72bcf
Compare
…tor-contrib into githubreceiver-actions-traces
| @@ -24,6 +25,7 @@ type Config struct { | |||
| scraperhelper.ControllerConfig `mapstructure:",squash"` | |||
| Scrapers map[string]internal.Config `mapstructure:"scrapers"` | |||
| metadata.MetricsBuilderConfig `mapstructure:",squash"` | |||
| Traces traces.Config `mapstructure:"traces"` | |||
There was a problem hiding this comment.
@Elfo404 - This is what I was talking about. Could you give some purview on the complications or ease of backend logic that is able so separate events that are built into traces vs events that could be built into metrics of just sent as raw events?
My thinking here, and this could come in a subsequent iteration, is that it'd be simpler to only require the configuration of one GitHub app to sent all data, instead of requiring the configuration of multiple to point to different hooks that process logs and traces.
Also curious your thoughts here @TylerHelmuth
There was a problem hiding this comment.
yeah, I was looking at a previous revision which called it Webhook.
I think keeping it simple from a config POV is better (i.e. reverting back to calling this webhook). Reason being you wouldn't have to manage multiple GH apps. (which if needed for other reasons you can still do by having different instances of the receiver with different config, for instance because of rate limits). Then we can add different config sections to customize the behavior of how traces or metrics are generated.
Internally is pretty straightforward to differentiate between events; take a look at what we did in https://github.com/grafana/grafana-ci-otel-collector/blob/main/receiver/githubactionsreceiver/receiver.go#L229, where we differentiate on the event type, and generate metrics on non-skipped workflow_job events, and after generate traces and logs on completed workflow_job and workflow_run events (https://github.com/grafana/grafana-ci-otel-collector/blob/main/receiver/githubactionsreceiver/receiver.go#L257-L293).
it's a bit of spaghetti code, but the metrics bit was thrown together in very little time. I'm sure that with some routing logic we can keep the implementation clean and not require users to define different webhooks.
TLDR; my thought is that the configuration should be independent on how the event handling is implemented, the fact that traces are generated thanks to the webhook is an implementation detail. WDYT?
| @@ -0,0 +1,168 @@ | |||
| # GitHub Actions Receiver | |||
|
|
|||
| <!-- status autogenerated section --> | |||
There was a problem hiding this comment.
This whole auto generated section can be deleted.
I think most of the information in this readme should be in the readme at the root of the receiver, and if necessary, can link to this as an internal document after adjustement.
| var _ component.Config = (*Config)(nil) | ||
|
|
||
| // Validate checks the receiver configuration is valid | ||
| func (cfg *Config) Validate() error { |
There was a problem hiding this comment.
TODO: revisit this after the discussion of using the webhook receiver as part of this or re-implementing similar code.
Co-authored-by: Adriel Perkins <adrielp@liatrio.com>
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
My work has taken a turn away from this, and I'm afraid I probably can't continue here. Would anyone like to pick up this work, or shall I close this PR? |
Firstly, thanks for the work on this @msarahan. Secondly thanks for letting us know. I'll try to pick up this work from here. I know contributions can take time, but if you ever have bandwidth again, feel free to contribute, always happy to see new folks contributing! |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
This is an initial skeleton of work adapted from @krzko from the discussion in #27460. It has much/most of the implementation details left out, but the general idea is to refactor what krzko had into a folder in
internal, and plumb the configuration in with the top-level github (scraper) configuration.I think we will want to remove the restriction that at least one scraper must be configured, since this receiver could be used with the GHA tracing and not the metrics.
CC @adrielp - not sure if this is what you had in mind for a skeleton, but I hope it is helpful. If you have more specific directions for what you'd like to see, let me know.