Skip to content

chore(data-service): add a log whenever connecting throws and include connection id COMPASS-9404#6953

Merged
nbbeeken merged 6 commits intomainfrom
compass-log-improvements
May 23, 2025
Merged

chore(data-service): add a log whenever connecting throws and include connection id COMPASS-9404#6953
nbbeeken merged 6 commits intomainfrom
compass-log-improvements

Conversation

@nbbeeken
Copy link
Collaborator

@nbbeeken nbbeeken commented May 22, 2025 •
edited
Loading

Description

https://jira.mongodb.org/browse/COMPASS-9404

  • Adds a new log message in a catch block.
  • Adds connection id as an attr to connecting, success, and failure

Checklist

  • New tests and/or benchmarks are included
  • Documentation is changed or added
  • If this change updates the UI, screenshots/videos are added and a design review is requested
  • I have signed the MongoDB Contributor License Agreement (https://www.mongodb.com/legal/contributor-agreement)

Motivation and Context

  • Bugfix
  • New feature
  • Dependency update
  • Misc

Open Questions

Dependents

Types of changes

  • Backport Needed
  • Patch (non-breaking change which fixes an issue)
  • Minor (non-breaking change which adds functionality)
  • Major (fix or feature that would cause existing functionality to change)

nbbeeken
nbbeeken commented May 22, 2025
View reviewed changes
packages/data-service/src/data-service.ts
this._isConnecting = true;

this._logger.info(mongoLogId(1_001_000_014), 'Connecting', {
this._logger.info(mongoLogId(1_001_000_014), 'Connecting Started', {
Copy link
Collaborator Author

@nbbeeken nbbeeken May 22, 2025

Choose a reason for hiding this comment

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

I am inspired here by the driver's commandStarted/Succeeded/Failed. But the message changes aren't necessary so happy to undo.

Copy link
Collaborator

@gribnoysup gribnoysup May 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

If the log id stays the same and conceptually it's the same event I think that's fine (but you will need to figure out all the tests that might be checking this 🙂)

packages/data-service/src/data-service.ts

this._logger.info(mongoLogId(1_001_000_014), 'Connecting', {
this._logger.info(mongoLogId(1_001_000_014), 'Connecting Started', {
connectionId: this._id,
Copy link
Collaborator Author

@nbbeeken nbbeeken May 22, 2025

Choose a reason for hiding this comment

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

IIUC this _id is per attempt not per cluster, but that's fine for the use case in mms. LMK if "connectionId" isn't clear, I think its just important to not conflate that with a clusterId of some kind

@nbbeeken nbbeeken marked this pull request as ready for review May 22, 2025 21:03
@nbbeeken nbbeeken requested a review from gribnoysup May 22, 2025 21:03
@nbbeeken
Copy link
Collaborator Author

nbbeeken commented May 22, 2025

What constitutes a fix/feat in compass? also happy to file a ticket for this for the history of it all. TIA!

@nbbeeken nbbeeken changed the title chore(data-service): add a log whenever connecting throws and include connection id chore(data-service): add a log whenever connecting throws and include connection id COMPASS-9404 May 22, 2025
@gribnoysup
Copy link
Collaborator

gribnoysup commented May 23, 2025 •
edited
Loading

The "compass-e2e-tests__Logging_and_Telemetry_integration_after_running_an_example_path_through_Compass_log_events_for_the_critical_path.logs__Connecting_Succeeded" failure in CI looks legit (and we should have fixes in soon for other failures to make it less noisy), otherwise looks good!

What constitutes a fix/feat in compass? also happy to file a ticket for this for the history of it all. TIA!

We don't have very strict guides on this, fix-es are typically for anything that can be qualified as a bug (not necessarily user-facing, can be something in build tooling for example), feat-s are usually user-facing changes in Compass, for everything else we default to chore, which is. probably a good enough match for this

@nbbeeken nbbeeken force-pushed the compass-log-improvements branch from d3813b8 to 77d1208 Compare May 23, 2025 13:47
@nbbeeken nbbeeken merged commit dc087d0 into main May 23, 2025
72 of 76 checks passed
@nbbeeken nbbeeken deleted the compass-log-improvements branch May 23, 2025 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gribnoysup gribnoysup gribnoysup approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@nbbeeken @gribnoysup