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

crypto: Include megolm ratchet index in logging span fields #3989

Merged
merged 1 commit into from
Sep 13, 2024

Conversation

richvdh
Copy link
Member

@richvdh richvdh commented Sep 12, 2024

This field is helpful as it tells us the sequence number of the message in the megolm session, which gives us a clue about how long it will have been since the session should have been shared with us.

[Question: an alternative would be just to attach the whole of content to the span, instead of separate fields for sender_key, session_id, message_index. The Debug defintion for MegolmV1AesSha2Content looks like it contains the right things, so the code might be simpler. On the other hand, I think the output would be less readable. Thoughts?]

This field is helpful as it tells us the sequence number of the message in the
megolm session, which gives us a clue about how long it will have been since
the session should have been shared with us.
Copy link

codecov bot commented Sep 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.32%. Comparing base (2408df8) to head (6901efe).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3989      +/-   ##
==========================================
+ Coverage   84.27%   84.32%   +0.05%     
==========================================
  Files         266      266              
  Lines       28296    28299       +3     
==========================================
+ Hits        23847    23864      +17     
+ Misses       4449     4435      -14     

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

@poljar
Copy link
Contributor

poljar commented Sep 12, 2024

[Question: an alternative would be just to attach the whole of content to the span, instead of separate fields for sender_key, session_id, message_index. The Debug defintion for MegolmV1AesSha2Content looks like it contains the right things, so the code might be simpler. On the other hand, I think the output would be less readable. Thoughts?]

I think I prefer separate fields, since the content might contain unknown fields somebody might include something they might not want to end up in logs or might contain spam.

@richvdh richvdh marked this pull request as ready for review September 13, 2024 11:18
@richvdh richvdh requested review from a team as code owners September 13, 2024 11:18
@richvdh richvdh requested review from bnjbvr and andybalaam and removed request for a team September 13, 2024 11:18
@richvdh richvdh merged commit 72cc2bd into main Sep 13, 2024
55 checks passed
@richvdh richvdh deleted the rav/log_megolm_index branch September 13, 2024 11:18
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