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 play-json dependency #55

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Remove play-json dependency #55

wants to merge 1 commit into from

Conversation

gheine
Copy link
Contributor

@gheine gheine commented Dec 19, 2018

No description provided.

Copy link
Contributor

@ericluria ericluria left a comment

Choose a reason for hiding this comment

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

Code looks good. How does the data look in Rollbar using the default serializer? I recall in the past that there were extra quotes showing up in logged data, which is why I think we tried using Play JSON.

I think you can test locally in sbt console using any project that has Rollbar enabled, as long as you have the ROLLBAR_TOKEN env variable set.

@@ -140,7 +94,6 @@ object RollbarProvider {
.sender(
new SyncSender.Builder()
.accessToken(token)
.jsonSerializer(jacksonSerializer)
.build()
Copy link
Contributor

Choose a reason for hiding this comment

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

check to see if we can use the default sender here (i.e. not call it because the defaults are the same)

val mapper = new ObjectMapper()

// de/serialize play-json types
mapper.registerModule(PlayJsonModule)
Copy link
Contributor

@benwaffle benwaffle Dec 21, 2018

Choose a reason for hiding this comment

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

Removing PlayJsonModule this means that we can no longer serialize Js* types (e.g. JsObject). I feel like being able to log objects is important for richer structured log messages. It would be good to determine whether any flow code logs any Js* values

@benwaffle
Copy link
Contributor

Why do we want to remove the play-json dep? Is there a new project with which we want to use lib-log but not play?

Base automatically changed from master to main March 23, 2021 14:10
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