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

warn on missing unit for duration/size config vars, require unit on new config vars #2121

Closed
trentm opened this issue Jun 22, 2021 · 3 comments
Assignees
Labels
agent-nodejs Make available for APM Agents project planning.
Milestone

Comments

@trentm
Copy link
Member

trentm commented Jun 22, 2021

There was a discussion recently and suggestion from @felixbarny that the APM agents make the units on time/duration config vars (e.g. metricsInterval) required instead of optional. The history there is that some Java agent config vars that were later shared with other agents (e.g. span_frames_min_duration) used to have the units in the name (e.g. span_frames_min_duration_ms). The switch to drop the units in the name added support for units in the value, but for backward compatibility supported default units.

The problem: There are some cases where the user can get surprised:

  • different default units between agents (e.g. the recently added spanFramesMinDuration in the node.js agent defaults to seconds with no units, but the default is milliseconds for other agents)
  • the default units are not obvious to the user doing the configuration

Making the units required is obviously a breaking change, so would need to wait for a major version bump.

(See also the related #2120 for handling invalid config values.)

@trentm trentm added this to the next-major milestone Jun 22, 2021
@github-actions github-actions bot added the agent-nodejs Make available for APM Agents project planning. label Jun 22, 2021
@trentm
Copy link
Member Author

trentm commented Jul 26, 2022

There was an earlier issue for this as well: #1504

@trentm
Copy link
Member Author

trentm commented Jul 25, 2023

I propose to avoid a breaking change here. In 4.x we would:

  • add a warning for duration and size options that don't provide an explicit unit
  • add support for and make any new duration and size options have the unit be required.

@trentm trentm changed the title make unit on time/duration config vars *required* warn on missing unit for duration/size config vars, require unit on new config vars Jul 25, 2023
@trentm trentm self-assigned this Jul 25, 2023
@trentm trentm removed their assignment Aug 1, 2023
@david-luna david-luna self-assigned this Aug 3, 2023
trentm added a commit that referenced this issue Aug 4, 2023
@trentm
Copy link
Member Author

trentm commented Aug 4, 2023

This was handled in #3553 and #3544 on the "dev/4.x" branch, which will be the active 4.x release branch soonish.

@trentm trentm closed this as completed Aug 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agent-nodejs Make available for APM Agents project planning.
Projects
None yet
Development

No branches or pull requests

2 participants