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

Add move locator to rate engine log messages: MB-1399 #3463

Merged
merged 12 commits into from Feb 5, 2020

Conversation

@lynzt
Copy link
Contributor

lynzt commented Feb 4, 2020

Description

Add the move locator to the log messages of the rate engine for easier debugging and searching logs.

Setup

make server_run
make client_run
create a move and when you get to the screen that has the estimator (the weight slider), look at your server output and you should see things like:

2020-02-04T17:14:10.306Z INFO rateengine/nonlinehaul.go:191 sit calculation { moveLocator: B96BKK, moreDetails }
2020-02-04T17:14:10.306Z INFO rateengine/rateengine.go:158 PPM cost computation { moveLocator: B96BKK, moreDetails }

in this case, the move locator is B96BKK

Code Review Verification Steps

  • Code follows the guidelines for Logging
  • Tested in the Experimental environment (for changes to containers, app startup, or connection to data stores)
  • User facing changes have been reviewed by design.
  • Request review from a member of a different team.
  • Have the Jira acceptance criteria been met for this change?

References

Copy link
Contributor

chrisgilmerproj left a comment

I'm going to request that you remove AppendID from this PR and instead use zap.String("LocatorID", moveID) as an additional zapField in all the places. This means that in the CloudWatch logs you will be able to search on LocatorID and filter results. The way you have it now the result searching is going to be a lot more difficult in cloudwatch.

@lynzt

This comment has been minimized.

Copy link
Contributor Author

lynzt commented Feb 4, 2020

I have no problem using... zap.String("LocatorID", moveID)
I addled it to the msg since I knew for we can search on that field and it worked, but I'm find putting it in the { } but I'll test first...
I also want to test on experimental before merging...

@lynzt

This comment has been minimized.

Copy link
Contributor Author

lynzt commented Feb 4, 2020

@chrisgilmerproj oh, that's much better - just tested and def. going to update this! thanks

@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor

chrisgilmerproj commented Feb 4, 2020

@chrisgilmerproj oh, that's much better - just tested and def. going to update this! thanks

Oh good! Also, do you want to update the examples in the description of the PR?

Copy link
Contributor

chrisgilmerproj left a comment

🚀 - Thanks for making updates for me :) You may still want someone else to take a look but it seems fine to me.

Copy link
Contributor

garrettqmartin8 left a comment

👍

@lynzt lynzt merged commit 4a04e0f into master Feb 5, 2020
15 checks passed
15 checks passed
ci/circleci: acceptance_tests_experimental Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_local Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_staging Your tests passed on CircleCI!
Details
ci/circleci: build_app Your tests passed on CircleCI!
Details
ci/circleci: build_migrations Your tests passed on CircleCI!
Details
ci/circleci: build_storybook_app Your tests passed on CircleCI!
Details
ci/circleci: build_tasks Your tests passed on CircleCI!
Details
ci/circleci: build_tools Your tests passed on CircleCI!
Details
ci/circleci: check_generated_code Your tests passed on CircleCI!
Details
ci/circleci: client_test Your tests passed on CircleCI!
Details
ci/circleci: integration_tests Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_golang Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_yarn Your tests passed on CircleCI!
Details
ci/circleci: pre_test Your tests passed on CircleCI!
Details
ci/circleci: server_test Your tests passed on CircleCI!
Details
@lynzt lynzt changed the title Add move locator to rate engine log messages Add move locator to rate engine log messages: MB-1399 Feb 11, 2020
@lynzt lynzt deleted the lt-ppm-rateengine-logging branch Feb 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.