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

Breadcrumbs/Mongo: Remove 'started' breadcrumb, add collection detail #519

Merged
merged 5 commits into from
Jan 7, 2019

Conversation

Cawllec
Copy link
Contributor

@Cawllec Cawllec commented Jan 3, 2019

Goal

Removes the "started" breadcrumb, while adding the commands collection name.

Adds filter keys in the format: {"filter_1":"?","filter_2":"?"} etc.

Adds new and improves existing end-to-end tests for mongo breadcrumbs.

@Cawllec Cawllec changed the title Breadcrumbs/Mongo: Remove 'started' breadcrumb, add collection detail WIP: Breadcrumbs/Mongo: Remove 'started' breadcrumb, add collection detail Jan 3, 2019
@Cawllec Cawllec requested review from kellymason, tobyhs and a team January 3, 2019 16:59
@Cawllec Cawllec changed the title WIP: Breadcrumbs/Mongo: Remove 'started' breadcrumb, add collection detail Breadcrumbs/Mongo: Remove 'started' breadcrumb, add collection detail Jan 3, 2019
end

def pop_command(request_id)
events.delete(request_id) { nil }
Copy link
Contributor

Choose a reason for hiding this comment

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

Hash#delete returns nil if the key is not found, so the { nil } is unnecessary

events.delete(request_id) { nil }
end

def events
Copy link
Contributor

Choose a reason for hiding this comment

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

This method name (events) seems a little misleading. Perhaps it should be called something like commands or event_commands.

}
meta_data[:duration] = event.duration if defined?(event.duration)
if command = pop_command(event.request_id)
meta_data[:collection] = command[event.command_name]
Copy link
Contributor

Choose a reason for hiding this comment

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

For getMore commands, the collection name is in command['collection'] (and not command['getMore'])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's annoyingly inconsistent. I'll have a quick check to see if there are any other exceptions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From looking through their test specifications here, it looks like getMore is the only one.

Copy link
Contributor

@tobyhs tobyhs left a comment

Choose a reason for hiding this comment

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

a couple small comments, otherwise lgtm

meta_data[:collection] = command[collection_key]
unless command["filter"].nil?
filters = command["filter"].map { |key, _v| [key, '?'] }.to_h
meta_data[:filters] = JSON.dump(filters)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would call this :filter (singular instead of plural) as it seems to closer align with what the mongo gem and MongoDB documentation uses.

#
# @param request_id [String] the id of the mongo_ruby_driver event
#
# @return [Hash|nil] the requested command, or nil if not found
Copy link
Contributor

Choose a reason for hiding this comment

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

[Hash, nil] (as I don't think a | is valid in a yardoc type description)

@Cawllec Cawllec merged commit 4bd4127 into breadcrumbs/base Jan 7, 2019
@Cawllec Cawllec deleted the breadcrumbs/mongo-started-improvements branch January 7, 2019 16:04
Cawllec added a commit that referenced this pull request Jan 17, 2019
* Breadcrumbs/Circular buffer: Added circular buffer class

* Breadcrumbs/Circular buffer: Added initial buffer unit tests

* Breadcrumbs/Circular buffer: Ensure test file is named correctly

* Breadcrumbs/Circular buffer: Code tidy up

* Breadcrumbs / Breadcrumb class: Add first pass breadcrumb class with unit tests

* Breadcrumbs/Circular buffer rubocop fixes

* Breadcrumbs / Breadcrumb class: Rubocop fixes

* Breadcrumbs/Cricular buffer: Improve test readability

* Breadcrumbs/Breadcrumb: Simplified timestamp, improved tests

* Breadcrumbs/Breadcrumb: Rubocop requirements

* Breadcrumbs/Breadcrumb: Added doc comments

* Breadcrumbs/Circular buffer: Fixed RSpec/Rspec typo

* Breadcrumbs/Breadcrumb: Using explicit RSpec.describe call

* Breadcrumbs/Validator: Added validator and tests

* Breadcrumbs/Validator: Rubocop fixes

* Breadcrumbs/Validator: Tidying up from initial review

* Breadcrumbs/Validator: Re-add renamed types file

* Breadcrumbs/Breadcrumb: Change message => name, add yard comments

* Breadcrumbs/Circular buffer: Added yard docs

* Breadcrumbs/Breadcrumb: Improve tests, convert to utc sooner

* Breadcrumbs/Configuration: Initial configuration changes

* Breadcrumbs/Report: Initial report changes to support breadcrumbs

* Breadcrumbs/Breadcrumb: Test timestamps match up

* Breadcrumbs/Validator: Change 'message' to 'name', use name and meta_data clones

* Breadcrumbs/Validator: Refactor toward instance_double & let

* Breadcrumbs/Configuration: Add in missing import statement

* Breadcrumbs/Circular buffer docs: Improve readability

* Breadcrumbs/Configuration: Add configuration tests

* Breadcrumbs/Configuration: Added yardoc comments

* Breadcrumbs/Configuration: Simplify before_breadcrumb_callbacks, allow automatic_breadcrumb_types editting

* Breadcrumbs/Configuration: Add automatic breadcrumbs types editablilty test

* Breadcrumbs/Configuration: Add types to Yardoc arrays

* Breadcrumbs/Validator: Use an instance_double to reflect Configuration better

* Breadcrumbs/Validator: Add Yardoc comments

* Breadcrumbs/Validator: Use select over each when filtering

* Breadcrumbs/Validator: Clean up tests

* Breadcrumbs/Validator: Use double for configuration

* Breadcrumbs/Validator: Improved Yardoc

* Breadcrumbs/Leave breadcrumb: Add implementation and tests

* Breadcrumbs/Middleware: Add breadcrumb middleware

* Breadcrumbs/Report: Rubify get_summary method name

* Breadcrumbs/Report: Ensure message is set correctly

* Breadcrumbs/Report: Change default breadcrumbs from Hash to Array

* Breadcrumbs/Report: Ensure breadcrumb meta_data is filtered correctly

* Breadcrumbs/Report: Fixed typo

* Bugsnag/Report: Added tests

* Breadcrumbs/Leave breadcrumb: Update rubocop to allow increased Bugsnag module length

* Breadcrumbs/middleware: Rubocop fixes

* Breadcrumbs/Leave-breadcrumb: Rubocop improvements/early return

* Breadcrumbs/Leave-breadcrumb: Better block styling

* Breadcrumbs/Report: Ensure report summary is consistent with dashboard

* Breadcrumbs/Report: Better Rubocop report exclusions

* Breadcrumbs/Report: Rspec improvements

* Breadcrumbs/Notify-breadcrumb: Initial implementation and tests

* Breadcrumbs/Notify breadcrumb: Add test against notifying a string

* Breadcrumbs/Report: Better checking against Java error

* Breadcrumbs/Report: Make summary more accurate

* Breadcrumbs/Report update: make summary guard more readable

* Breadcrumbs/Update automatic_breadcrumb_types to enabled_automatic_breadcrumb_types

* Breadcrumbs: Rails Breadcrumbs (#514)

* Breadcrumbs/Rails integration: Initial integration implemenetation

* Breadcrumbs/Rails: Minor fixes

* Breadcrumbs/Rails: Add initial maze tests

* Breadcrumbs/Rails breadcrumbs: Remove plural event data

* Breadcrumbs/Rails breadcrumbs: Ammend breadcrumb namespace

* Breadcrumbs/Rails breadcrumbs: Remove event_id from allowed keys, change name to event_name

* Breadcrumbs/Rails breadcrumbs: Require breadcrumb file when necessary to avoid namespace clashes

* Breadcrumbs/Rails breadcrumbs: Fix rubocop issues

* Breadcrumbs/Rails breadcrumbs: Clarify namespaces better

* Breadcrumbs/Rails breadcrumbs: Remove invalid data from captured breadcrumb definitions

* Breadcrumbs/Rails breadcrumbs: Added ActiveJob breadcrumb test

* Breadcrumbs/Rails breadcrumbs: Ammended active_job event id

* Breadcrumbs/Ruby breadcrumbs: Removed complex breadcrumb metadata

* Breadcrumbs/Ruby: Added cache breadcrumb

* Breadcrumbs/Ruby breadcrumbs: Fleshed out maze-test expectations where possible

* Breadcrumbs/Rails breadcrumbs: Fixed broken test fixture, made fixtures more rails-y

* Breadcrumbs: Add Mongo integration (#516)

* Breadcrumbs/Rails integration: Initial integration implemenetation

* Breadcrumbs/Rails: Minor fixes

* Breadcrumbs/Rails: Add initial maze tests

* Breadcrumbs/Mongo: Initial subscriber implementation

* Breadcrumbs/Rails breadcrumbs: Remove plural event data

* Breadcrumbs/Rails breadcrumbs: Ammend breadcrumb namespace

* Breadcrumbs/Rails breadcrumbs: Remove event_id from allowed keys, change name to event_name

* Breadcrumbs/Rails breadcrumbs: Require breadcrumb file when necessary to avoid namespace clashes

* Breadcrumbs/Mongo integration: Fix module name issue

* Breadcrumbs/Mongo integration: Maintain consistency with ActiveSupport arguments

* Breadcrumbs/Rails breadcrumbs: Fix rubocop issues

* Breadcrumbs/Rails breadcrumbs: Clarify namespaces better

* Breadcrumbs/Mongo integration: Add integration tests

* Breadcrumbs/Mongo integration: Remove example commits

* Breadcrumbs/Rails breadcrumbs: Remove invalid data from captured breadcrumb definitions

* Breadcrumbs/Mongo integration: Rubocop fixes

* Breadcrumbs/Mongo integration: Better doc comments

* Breadcrumbs/Mongo integration: Improve tests, add 'failure' test

* Breadcrumbs/Mongo integration: Re-add 'success' test

* Breadcrumbs/Rails breadcrumbs: Added ActiveJob breadcrumb test

* Breadcrumbs/Rails breadcrumbs: Ammended active_job event id

* Breadcrumbs/Ruby breadcrumbs: Removed complex breadcrumb metadata

* Breadcrumbs/Ruby: Added cache breadcrumb

* Breadcrumbs/Ruby breadcrumbs: Fleshed out maze-test expectations where possible

* Breadcrumbs/Mongo integration: Rubocop fixes

* Breadcrumbs/Rails breadcrumbs: Fixed broken test fixture, made fixtures more rails-y

* Breadcrumbs/Mongo integration: Maze fixes

* Breadcrumbs/Validator: Allow nil as an acceptable meta_data type (#520)

* Breadcrumbs/Mongo: Remove 'started' breadcrumb, add collection detail (#519)

* Breadcrumbs/Mongo integration: Remove 'started' event, add 'collection' data

* Breadcrumbs/Mongo: Add filter keys

* Breadcrumbs/Mongo integration: Made filters consistent with other breadcrumb implementations

* Breadcrumbs/Mongo integration: Rubocop fixes

* Breadcrumbs/Mongo integration: Address feedback

* Breadcrumbs: Additional Rails ActiveRecord data (#521)

* Breadcrumbs/Rails breadcrumbs: Capture event_id

* Breadcrumbs/Rails breadcrumbs: Capture 'sql' string and redacted bindings

* Breadcrumbs/Rails integration: Redact SQL string values

* Breadcrumbs/Rails integration: Remove SQL string acquisition

* Breadcrumbs/Rails improvements: Remove unnecessary require

* Breadcrumbs/Rails integration: Add integration tests for bindings

* Breadcrumbs/Ruby integration: lock nokogiri version

* Breadcrumbs/Ruby integration: Better meta_data naming, Ruby < 2.1 compatability

* Breadcrumbs/Mongo integration: Handle 'any_of' cases (#522)

* Breadcrumbs/Mongo integration: Handle 'any_of' cases

* Breadcrumbs/Mongo Integration: Improve filter sanitization process and add unit tests

* Breadcrumbs/Mongo integration: Ensure tests are notated correctly

* Breadcrumbs/Mongo filters: Add 1.9.3 & 2.0.0 compatability

* Breadcrumbs/Mongo integration: Appease rubocop

* Breadcrumbs/Mongo integration: Minor test and efficieny improvements

* Breadcrumbs/Mongo integration: Test improvements

* Breadcrumbs/Mongo integrations: Test spelling

* Breadcrumbs: Fix rubocop issues (excluding configuration from ClassLength until refactor)
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