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

feat: LogArgsSerializer to customize how args are converter to string #580

Merged
merged 7 commits into from
May 2, 2024

Conversation

mxab
Copy link
Contributor

@mxab mxab commented May 2, 2024

Why

The default behaviour in pushLogs stringifies all arguments in a very simple way. This results in potential [object Object] log messages if some one or some third party library decides to put complex logs arguments e.g. console.info({foo: "bar"})

What

This PR extends the faro config with a logArgsSerializer paremeter that allows to override the default behaviour and put in a custom args renderer

fixes #564

Checklist

  • Tests added
  • Changelog updated
  • Documentation updated

@CLAassistant
Copy link

CLAassistant commented May 2, 2024

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

CHANGELOG.md Outdated Show resolved Hide resolved
@codecapitano
Copy link
Collaborator

codecapitano commented May 2, 2024

Hi @mxab can you also add the new property to the core readme please.

Section: Besides the mandatory properties, Faro configuration also supports the following optional properties:

mxab and others added 3 commits May 2, 2024 15:25
Co-authored-by: Marco Schaefer <47627413+codecapitano@users.noreply.github.com>
@mxab
Copy link
Contributor Author

mxab commented May 2, 2024

Done, do you want me to squash the changes?

@codecapitano
Copy link
Collaborator

Done, do you want me to squash the changes?

Hey Max you don't need to squash them yourself.
We'll squash & merge them via GH.

Copy link
Collaborator

@codecapitano codecapitano left a comment

Choose a reason for hiding this comment

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

@mxab Great addition thank you so much. 🙏
Thanks for opening the initial issue and implementing the solution as well 🥳

@mxab
Copy link
Contributor Author

mxab commented May 2, 2024

yeah I only executed the linter in the core package. thx

@mxab
Copy link
Contributor Author

mxab commented May 2, 2024

at last the linter approves 😅

@codecapitano codecapitano merged commit e7bc2f9 into grafana:main May 2, 2024
2 checks passed
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.

pushLog argument serializer
3 participants