Skip to content

Bug 2020855 - add timestamps to breadcrumb data#7279

Merged
bendk merged 1 commit intomozilla:mainfrom
bendk:push-ovnurnmnylss
Mar 20, 2026
Merged

Bug 2020855 - add timestamps to breadcrumb data#7279
bendk merged 1 commit intomozilla:mainfrom
bendk:push-ovnurnmnylss

Conversation

@bendk
Copy link
Contributor

@bendk bendk commented Mar 19, 2026

Also removed the tracing dependency, which was optional and never used. All calls go through tracing-support.

Pull Request checklist

  • Breaking changes: This PR follows our breaking change policy
    • This PR follows the breaking change policy:
      • This PR has no breaking API changes, or
      • There are corresponding PRs for our consumer applications that resolve the breaking changes and have been approved
  • Quality: This PR builds and tests run cleanly
    • Note:
      • For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
      • If this pull request includes a breaking change, consider cutting a new release after merging.
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry in CHANGELOG.md or an explanation of why it does not need one
    • Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
  • Dependencies: This PR follows our dependency management guidelines
    • Any new dependencies are accompanied by a summary of the due diligence applied in selecting them.

@bendk bendk requested review from a team and mhammond March 19, 2026 14:57
@bendk bendk force-pushed the push-ovnurnmnylss branch 2 times, most recently from ac36e37 to beb7c2d Compare March 19, 2026 15:02
tracing_support::info!(target: "app-services-error-reporter::breadcrumb", message, module, line, column);
}

fn add_breadcrumb_timestamp(message: &str, time: SystemTime) -> String {
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if format might be a better prefix than add? I first expected to see this "add the breadcrumb" and was confused why the caller did the push. Not a bug deal though.

Also, is it at all possible we will want to order or group by timestamps? Should the timestamp maybe be appended instead, maybe inside parens?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the function name and put the timestamp at the end, both of those seem like solid improvements to me. I don't understand the group by timestamp part, how would that work?

Also removed the `tracing` dependency, which was optional and never
used. All calls go through `tracing-support`.
@bendk bendk force-pushed the push-ovnurnmnylss branch from beb7c2d to 6757240 Compare March 20, 2026 13:19
@bendk bendk enabled auto-merge March 20, 2026 13:22
@bendk bendk added this pull request to the merge queue Mar 20, 2026
Merged via the queue into mozilla:main with commit e3497df Mar 20, 2026
14 checks passed
@bendk bendk deleted the push-ovnurnmnylss branch March 20, 2026 14:05
@mhammond
Copy link
Member

I don't understand the group by timestamp part, how would that work?

Sorry I was unclear - I meant that the timestamp at the end would allow simpler grouping/sorting via the name

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