Navigation Menu

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

🐛 remove readonly from all LogsEvent properties #1198

Merged

Conversation

kotarella1110
Copy link
Contributor

@kotarella1110 kotarella1110 commented Dec 7, 2021

Motivation

In some cases, I want to update the readonly LogsEvent properties by using beforeSend.

resolve #1195

Changes

remove readonly from LogsEvent all properties.

Testing

  • Local
  • Staging
  • Unit
  • End to end

I have gone over the contributing documentation.

@kotarella1110 kotarella1110 requested a review from a team as a code owner December 7, 2021 02:56
@bits-bot
Copy link

bits-bot commented Dec 7, 2021

CLA assistant check
All committers have signed the CLA.

@bcaudan
Copy link
Collaborator

bcaudan commented Dec 7, 2021

Hello @kotarella1110,

Thanks for this PR!
Actually, you could even remove all readonly from logsEvent properties to avoid similar issues to be raised.

readonly modifiers were introduced when only a handful of properties was editable, cf #644.
However, we decided to allow users to do what they want with log events in #909 but without updating types accordingly.

@kotarella1110
Copy link
Contributor Author

Actually, you could even remove all readonly from logsEvent properties to avoid similar issues to be raised.

@bcaudan I see. will do 👍🏻

@kotarella1110
Copy link
Contributor Author

kotarella1110 commented Dec 7, 2021

@bcaudan I think we should avoid updating the following properties, so it is better to leave the readonly property as it is.

  • date
  • application_id
  • service
  • session_id
  • view.id

Please let me know what you think.

@bcaudan
Copy link
Collaborator

bcaudan commented Dec 7, 2021

I think we should avoid updating the following properties

it is already possible in javascript, so I would say no reason to have the limitation only in typescript.

@kotarella1110
Copy link
Contributor Author

it is already possible in javascript, so I would say no reason to have the limitation only in typescript.

Thank for your reply! I will fix.

@kotarella1110
Copy link
Contributor Author

@bcaudan Removed readonly from all properties. Please review 🙏🏻

@kotarella1110 kotarella1110 changed the title 🐛 fix LogsEvent type to update readyonly property by using beforeSend callback 🐛 fix LogsEvent type to update all readyonly properties by using beforeSend callback Dec 7, 2021
@bcaudan bcaudan changed the title 🐛 fix LogsEvent type to update all readyonly properties by using beforeSend callback 🐛 remove readonly from all LogsEvent properties Dec 7, 2021
@BenoitZugmeyer BenoitZugmeyer merged commit 3aed00c into DataDog:main Dec 7, 2021
@BenoitZugmeyer
Copy link
Member

Thank you for your contribution! I'll make a new release shortly.

@kotarella1110 kotarella1110 deleted the fix/improve-LogsEvent-type branch December 17, 2021 12:55
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.

[Logs] allowed to use beforeSend to change the log level?
4 participants