Skip to content

Add initial Logging guides#776

Open
neuronull wants to merge 9 commits intomainfrom
neuronull/pm-30550/add-contributing-guidelines-logging
Open

Add initial Logging guides#776
neuronull wants to merge 9 commits intomainfrom
neuronull/pm-30550/add-contributing-guidelines-logging

Conversation

@neuronull
Copy link
Contributor

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-30550

📔 Objective

This PR ports a couple of internal confluence docs around our usage of Rust specific logging frameworks for Desktop Native, and general logging guidelines, into the contributing docs.

The language-agnostic guidelines are influenced by my own experience and only a small portion of that (so far) has been at Bitwarden. I welcome any suggestions/ideas.

One area I didn't go too in depth with in these proposed changes is that of security/privacy around logging sensitive data. That is a whole subject in itself and we had some internal chats about it, there are some areas in Desktop Native that I'm aware of which take some approaches to preventing developers from accidentally logging sensitive data. But I felt that could be a separate follow-up effort to get alignment on before writing any documentation about it.

Plan is to

  • get some feedback on these changes
  • discuss at Architecture meeting
  • open tickets to track adding the respective language specific guides for C# and TS, analogs to the Rust one added here.

📸 Screenshots

@neuronull neuronull self-assigned this Mar 2, 2026
@neuronull
Copy link
Contributor Author

Tagging @MGibson1 who had some great suggestions for the internal confluence docs that these changes originated from.

Also tagging @justindbaur as Matt mentioned you're looking into tracing for C#.

@neuronull neuronull marked this pull request as ready for review March 2, 2026 17:25
@neuronull neuronull requested a review from a team as a code owner March 2, 2026 17:25
@github-actions
Copy link
Contributor

github-actions bot commented Mar 2, 2026

Logo
Checkmarx One – Scan Summary & Details25b722ae-744a-400a-bb0a-789d84e70e64

Great job! No new security vulnerabilities introduced in this pull request

2026-02-27T21:33:38.451849Z INFO the_nebuchadnezzar::layer_three: follow the white rabbit. foo=Name: Morpheus
```

## Async
Copy link
Member

Choose a reason for hiding this comment

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

Please include how the level parameter of #[instrument] impacts contained logs of a higher level.

Copy link
Member

Choose a reason for hiding this comment

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

What I think is interesting and probably deserves a callout is that the instrument context is only applied when it's level is met.

I'm kind of surprised by this behavior since it reduces the context of log messages, but if you know about it, perhaps it can be used for good

Copy link
Contributor Author

@neuronull neuronull Mar 11, 2026

Choose a reason for hiding this comment

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

Yeah TBH, I hadn't used that level parameter for the instrument macro in the past, so when I was experimenting with it as part of this suggestion, it also took me by surprise. On the surface it seems like it'd be less useful than I'd assumed it would be. In my commit 31efbcf , I tried to make a case for when it might be useful, but it feels contrived.

If you still want to include the mention of it here in this guide, I will add a note as you specified, about calling out the behavior of when the level is met.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

edit: I made that change in ef07f1f

Copy link
Member

Choose a reason for hiding this comment

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

I think what you did is fine. The other option is to just suggest against it's usage.

Co-authored-by: Matt Gibson <mgibson@bitwarden.com>
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Mar 4, 2026

Deploying contributing-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: ef07f1f
Status: ✅  Deploy successful!
Preview URL: https://0438e1b6.contributing-docs.pages.dev
Branch Preview URL: https://neuronull-pm-30550-add-contr.contributing-docs.pages.dev

View logs

@neuronull neuronull requested a review from MGibson1 March 4, 2026 22:22
Copy link
Member

@MGibson1 MGibson1 left a comment

Choose a reason for hiding this comment

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

I think one more round on the items I replied to, they're not quite hitting what I think they should

@neuronull neuronull requested a review from MGibson1 March 11, 2026 20:08
Copy link
Member

@MGibson1 MGibson1 left a comment

Choose a reason for hiding this comment

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

Let's resolve that empty page and then we're good to go

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