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

Fix Beyla CTRL+C shutdown. Also simplify pipeline #256

Merged
merged 5 commits into from
Sep 5, 2023

Conversation

mariomac
Copy link
Contributor

@mariomac mariomac commented Aug 31, 2023

In preparation for task #249 . The change is not big but it affects multiple files, as it has changed an ubiquitous type.

First, we have movedtransform.HTTPRequestSpan to request.Span in its own package.

Before this PR, eBPF tracers could send either []HTTPRequestTrace or []HTTPInfo slices. That mean that the Pipeline input channel had to be of the type []any and internally added a Codec that transformed each object to HTTPRequestSpan (In the Pipes library jargon, a codec is an component that automagically is attached to transform data when two connected pipeline stages do not match the output->input types).

Now the codec has been removed and the input channel is of the []request.Span instead of the []any type. Now each Tracer implementor has the responsibility to convert its input type to request.Span type, so the pipeline stages don't need to know about the eBPF types.

This will facilitate inserting the Service Name from the tracers, as we have now a unified type. It also will slightly improve speed, as we have removed a goroutine continuously iterating a channel.

This PR also fixes the sigint/sigterm handling, allowing Beyla to stop again without requiring a sigkill (bug introduced in PR #243).

return i
}

func TestHTTPInfoParsing(t *testing.T) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not removed but moved to another file.

@mariomac mariomac added the bug label Sep 4, 2023
@mariomac mariomac changed the title Simplify pipeline Fix Beyla CTRL+C shutdown. Also simplify pipeline Sep 4, 2023
Copy link
Contributor

@grcevski grcevski left a comment

Choose a reason for hiding this comment

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

LGTM! Looks much cleaner now, thanks! I think we might need to change this again in the future if we ever add different event types, e.g. security log events, but we'll cross that bridge when we get there :)

@mariomac
Copy link
Contributor Author

mariomac commented Sep 5, 2023

@grcevski yes! I agree that at some point we'd need to expand Beyla for more event types. Another option is not make each event type to pass across the same pipeline but have their own pipeline, as there would be some nodes that wouldn't be useful in that case (for example, the log events maybe don't need the routes decoration node).

@mariomac mariomac merged commit d341235 into grafana:main Sep 5, 2023
@mariomac mariomac deleted the report-svc-name branch September 29, 2023 12:32
mattdurham pushed a commit to mattdurham/beyla that referenced this pull request Jan 22, 2025
* moved transform.HTTPRequestSpan to request.Span

* Removed converter as pipe codec

* reformat

* Bugfix: stop beyla with simple sigint/sigterm
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants