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: Truncate string slice attributes in OTLP labels #434

Merged
merged 1 commit into from
Feb 4, 2025

Conversation

ericywl
Copy link
Contributor

@ericywl ericywl commented Jan 27, 2025

❓ Why is this being changed

Related to elastic/apm-server#14716

πŸ§‘β€πŸ’» What is being changed

  • Truncate the strings in slices when setting label attributes

βœ… How to validate the change

Run test on APM server with the changes

@ericywl ericywl marked this pull request as ready for review January 27, 2025 07:30
@ericywl ericywl requested a review from a team as a code owner January 27, 2025 07:30
@ericywl ericywl changed the title feat: Truncate string slice attributes in OTLP labels fix: Truncate string slice attributes in OTLP labels Jan 27, 2025
@ericywl ericywl force-pushed the truncate-string-slice-attributes branch from 0265093 to d0ccb60 Compare January 27, 2025 08:05
Copy link
Contributor

@marclop marclop left a comment

Choose a reason for hiding this comment

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

Thanks!

@ericywl
Copy link
Contributor Author

ericywl commented Feb 4, 2025

Is there anything that needs to be done aside from just pressing the merge button?

@simitt
Copy link
Contributor

simitt commented Feb 4, 2025

Is there anything that needs to be done aside from just pressing the merge button?

@ericywl we usually use Squash and merge (avoid creating a merge commit). Please ensure a good commit message, ideally including some term to close the related github card. If there is a reason for keeping several commits in the history, then you can use Rebase and merge.
Other than that - just hit the button :)

@ericywl ericywl merged commit 811313e into elastic:main Feb 4, 2025
4 checks passed
@carsonip
Copy link
Member

carsonip commented Feb 4, 2025

Is there anything that needs to be done aside from just pressing the merge button?

As apm-data is a library, to incorporate this change into the projects (e.g. apm-server), we'll need to cut a release (in this case, a patch release) and bump the apm-data depenedency version (which is usually done by automation) in the projects. I'm not sure if we have docs on this process. We can sync up if you want to give it a go.

We should not close the apm-server issue until all of the above is carried out.

@ericywl
Copy link
Contributor Author

ericywl commented Feb 5, 2025

As apm-data is a library, to incorporate this change into the projects (e.g. apm-server), we'll need to cut a release (in this case, a patch release) and bump the apm-data depenedency version (which is usually done by automation) in the projects. I'm not sure if we have docs on this process. We can sync up if you want to give it a go.

Hey Carson, sure I will give it a try.

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.

5 participants