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.

Sign up for GitHub

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

Jump to bottom

Add JSON log #298

Merged
merged 1 commit into from
Dec 15, 2020
Merged

Add JSON log #298

merged 1 commit into from
Dec 15, 2020

Conversation

Azer0s
Copy link
Collaborator

@Azer0s Azer0s commented Apr 24, 2020 •
edited

Description

JSON Log is not only a nice to have, for many scenarios it is also an important requirement. The goal of this PR is to introduce two new kinds of loggers into RIG, as well as a new environment variable.

With LOG_TYPE, you should be able to control what kind of log RIG produces on STDOUT. This can either be erlang(this is the default and is set if the environment variable is not set), json (this is basic JSON, nothing too fancy here) and gcl (which is the Google Cloud Logger LogEntry format).

TODO

  • GCL
  • Maybe not use a 3rd party lib (?)
  • Changelog
  • Doc

What to look out for

Dear reviewer, I want you to

  • gain knowledge - I think it's important for you to know about the change.
  • check that the code works on your machine.
  • suggest implementation-code design/structure/readability improvements.
  • suggest test-code design/structure/readability improvements.
  • suggest documentation improvements.
  • suggest language/writing improvements.
  • help to find bugs (described below!).

Process

The goal is to improve not only the code in this PR but also our skills! The "rules":

  • The review is considered "done" as soon as all reviewers have added their review, and all their comments have been addressed.
  • For knowledge-sharing reviews, each reviewer should "approve" the PR after studying its content.
  • After the approval, the merge is concluded by the developer.

Have fun!

@Azer0s
Copy link
Collaborator Author

Azer0s commented Apr 24, 2020

FYI: GitHub behaves really weird for me. It shows me that I did 9 commits when I only did 1. The other commits are from an entirely different branch. When I try to merge this branch locally, everything behaves as intended and only the 1 commit gets merged in. So this might just be a visual bug.

@kevinbader
Copy link
Contributor

FYI: GitHub behaves really weird for me. It shows me that I did 9 commits when I only did 1. The other commits are from an entirely different branch. When I try to merge this branch locally, everything behaves as intended and only the 1 commit gets merged in. So this might just be a isual bug.

image

It looks like you've based c996dc0 on the vconnection branch. You've merged master into feature/json-log, but merging back into master would definitely introduce those additional commits. I suggest cherry-picking c996dc0 onto master.

@Azer0s
Copy link
Collaborator Author

Azer0s commented Apr 26, 2020

FYI: GitHub behaves really weird for me. It shows me that I did 9 commits when I only did 1. The other commits are from an entirely different branch. When I try to merge this branch locally, everything behaves as intended and only the 1 commit gets merged in. So this might just be a isual bug.

image

It looks like you've based c996dc0 on the vconnection branch. You've merged master into feature/json-log, but merging back into master would definitely introduce those additional commits. I suggest cherry-picking c996dc0 onto master.

Thanks for the tip! This (as you can see 😁) worked!

kevinbader
kevinbader reviewed Apr 30, 2020
View reviewed changes
lib/rig/application.ex Outdated
# TODO: Fix format; context: config.exs:94
# Or can I leave it like that? The only difference seems to
# be that the level and the module are switched
Logger.add_backend(:console)
Copy link
Contributor

Choose a reason for hiding this comment

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

So the config for console is not used here? You could try to set the values using Application.put_env, which should basically do the same..

It would be good to respect the config setting if possible, because it deduplicates the meta data.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What do you mean? I meant that the format specified in config.exs doesn't apply here. I can fix metadata just by setting :metadata to :all. I don't think I can set the format through an env var.

Copy link
Contributor

@kevinbader kevinbader May 6, 2020 •
edited

Choose a reason for hiding this comment

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

I didn't mean environment variables - see https://hexdocs.pm/elixir/Application.html#put_env/4

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant that the format specified in config.exs doesn't apply here. I can fix metadata just by setting :metadata to :all.

Why can't you apply the previous config here? Not sure what you mean..

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm...it appears to work now...for some reason...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Like...it produces the same result (log-wise) as the master branch

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@kevinbader Is this closable then?

Copy link
Contributor

Choose a reason for hiding this comment

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

please double check whether it actually works now ;) but yes, if it really works, sure

kevinbader
kevinbader reviewed Apr 30, 2020
View reviewed changes
mix.exs Outdated Show resolved Hide resolved
@kevinbader kevinbader added this to the 3.0.0 milestone May 4, 2020
@Azer0s
Copy link
Collaborator Author

Azer0s commented May 22, 2020

@kevinbader Shall I maybe add a paragraph in metrics-details.md?

@kevinbader
Copy link
Contributor

@Azer0s please merge with current master.

@kevinbader Shall I maybe add a paragraph in metrics-details.md?

metrics-details.md is about Prometheus metrics - what would you like to document there?

@kevinbader kevinbader changed the title WIP: Add JSON log Add JSON log Nov 12, 2020
kevinbader
kevinbader previously approved these changes Dec 15, 2020
View reviewed changes
@kevinbader @Azer0s
Support JSON and GCL log formats
cbbcefe 
Co-authored-by: Ariel Simulevski <Azer0s@users.noreply.github.com>
@kevinbader kevinbader merged commit f7d2bc5 into master Dec 15, 2020
@kevinbader kevinbader deleted the feature/json-log branch December 15, 2020 23:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevinbader kevinbader kevinbader approved these changes

@mmacai mmacai Awaiting requested review from mmacai

Assignees

@kevinbader kevinbader

Labels
enhancement
Projects
None yet
Milestone
3.0.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Azer0s @kevinbader