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

Beyla remove future deprecated fields #3035

Merged
merged 7 commits into from
Mar 31, 2025
Merged

Conversation

marctc
Copy link
Contributor

@marctc marctc commented Mar 19, 2025

PR Description

Prior change the stability of the component, we can already introduce some breaking changes that
are anticipated to happen in the future of Beyla. This PR addresses them.

Added some extra config validations taken from Beyla code.

PR Checklist

  • CHANGELOG.md updated
  • Documentation added
  • Tests updated
  • Config converters updated

@marctc marctc requested review from a team and clayton-cornell as code owners March 19, 2025 15:39
Copy link
Contributor

github-actions bot commented Mar 19, 2025

💻 Deploy preview deleted.

@marctc marctc requested a review from a team as a code owner March 19, 2025 15:43
@marctc marctc requested a review from mariomac March 19, 2025 15:43
@@ -40,16 +40,9 @@ You can use the following arguments with `beyla.ebpf`:
| ----------------- | -------- | ----------------------------------------------------------------------------------- | ------- | -------- |
| `debug` | `bool` | Enable debug mode for Beyla. | `false` | no |
| `enforce_sys_caps`| `bool` | Enforce system capabilities required for eBPF instrumentation. | `false` | no |
| `executable_name` | `string` | The name of the executable to match for Beyla automatically instrumented with eBPF. | `""` | no |
Copy link
Contributor

Choose a reason for hiding this comment

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

This component is in Public Preview. Instead of just deleting the docs for the args... should we instead mark them as deprecated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm deleting these fields precisely because the component is in public preview and AFAIK it's safe to make breaking changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. Was just asking to be sure :-)

Copy link
Contributor

@mariomac mariomac 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 addressing this!

I left some comments about removal vs deprecation but I understand your point about advancing the breaking change before the Beyla component is marked as stable.

@@ -10,6 +10,10 @@ internal API changes are not present.
Main (unreleased)
-----------------

### Breaking changes

- Removed `open_port` and `executable_name` from top level configuration of Beyla component. Removed `enabled` argument from `network` block. (@marctc)
Copy link
Contributor

Choose a reason for hiding this comment

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

As long as they are hidden/deprecated but not removed in Beyla, maybe we can just mark them as deprecate dand move this line to another section.

@@ -70,7 +74,7 @@ v1.7.3

### Breaking changes

- Fixed the parsing of selections, application and network filter blocks for Beyla
- Fixed the parsing of selections, application and network filter blocks for Beyla. (@raffaelroquetto)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a breaking change instead of a bugfix? Can we get more context about what functionality/data is actually broken @rafaelroquetto?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was a bugfix and a breaking change

Comment on lines 12 to 13
Port string `alloy:"open_port,attr,optional"`
ExecutableName string `alloy:"executable_name,attr,optional"`
Copy link
Contributor

Choose a reason for hiding this comment

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

I would keep them as long as they are accepted in Beyla, despite they are hidden.

Copy link
Contributor

@clayton-cornell clayton-cornell left a comment

Choose a reason for hiding this comment

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

The only doc input (re deleting vs marking deprecated) was addressed. So all good from the docs side

@marctc marctc merged commit 6afbe55 into main Mar 31, 2025
35 checks passed
@marctc marctc deleted the beyla_remove_deprecated_fields branch March 31, 2025 14:36
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