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

Setup rails for JSON logging #1568

Merged
merged 2 commits into from
Sep 24, 2024
Merged

Setup rails for JSON logging #1568

merged 2 commits into from
Sep 24, 2024

Conversation

ryanmelt
Copy link
Member

No description provided.

@ryanmelt ryanmelt requested a review from jmthomas September 24, 2024 15:59
Copy link

codecov bot commented Sep 24, 2024

Codecov Report

Attention: Patch coverage is 87.03704% with 7 lines in your changes missing coverage. Please review.

Project coverage is 77.03%. Comparing base (5e4b7a0) to head (ac102d1).
Report is 17 commits behind head on main.

Files with missing lines Patch % Lines
openc3/lib/openc3/operators/operator.rb 0.00% 4 Missing ⚠️
openc3-cosmos-cmd-tlm-api/config/application.rb 83.33% 1 Missing ⚠️
...nc3-cosmos-script-runner-api/config/application.rb 83.33% 1 Missing ⚠️
openc3/lib/openc3/utilities/logger.rb 97.36% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1568      +/-   ##
==========================================
+ Coverage   77.02%   77.03%   +0.01%     
==========================================
  Files         611      611              
  Lines       46000    46026      +26     
  Branches      800      802       +2     
==========================================
+ Hits        35431    35456      +25     
- Misses      10481    10482       +1     
  Partials       88       88              
Flag Coverage Δ
frontend 55.86% <ø> (+0.07%) ⬆️
python 84.61% <ø> (+<0.01%) ⬆️
ruby-api 48.98% <83.33%> (+0.12%) ⬆️
ruby-backend 82.80% <88.09%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

end
username ||= 'anonymous'
payload = log.payload
if payload
Copy link
Member

Choose a reason for hiding this comment

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

How does the payload get set?

Copy link
Member Author

Choose a reason for hiding this comment

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

Comes from the semantic logging gem changing Rails.

end
# This happens for a seperate exception log entry which we want to not include the backtrace a second time
if log.exception
message = "Exception was raised - #{log.exception.class}:#{log.exception.message}" unless message
Copy link
Member

Choose a reason for hiding this comment

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

Isn't log.message always set? So this will never be set? Also do we want the exception message to override the passed message? Seems like an exception is more important ... I'm not sure how log.exception even gets set though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not always. I found for some exception logs they just include the exception object, not a message.

data[:message] = message if message
data[:type] = type if type
data[:url] = url if url
data = data.merge(other) if other
Copy link
Member

Choose a reason for hiding this comment

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

We need to make sure we don't overlap keys in other but I think we control the complete other hash so it should be ok.

Copy link
Member Author

Choose a reason for hiding this comment

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

We control other, so are capable of not reusing any names.

Copy link

@ryanmelt ryanmelt merged commit b0383ab into main Sep 24, 2024
27 checks passed
@ryanmelt ryanmelt deleted the json_rails_logging branch September 24, 2024 19:37
@jmthomas
Copy link
Member

@ryanmelt do we need to make any equivalent Python code changes here? I see you added other to the logger signature and changed operator which both have Python equivalents.

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.

2 participants