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

Support for arbitrary filters #2105

Merged
merged 1 commit into from May 9, 2019

Conversation

2 participants
@pjdufour-truss
Copy link
Contributor

commented May 8, 2019

Description

This PR adds the ability to search ECS service logs (aka application logs) with an arbitrary filter. Supports larger trace id work.

Reviewer Notes

Just try it out! I'd suggest searching by referrer or message.

Setup

Add any steps or code to run in this section to help others prepare to run your code:

make clean
make build_tools
ecs-service-logs show -c app-staging -s app referer=https://my.staging.move.mil/

Code Review Verification Steps

  • Code follows the guidelines for Logging
  • The requirements listed in
    Querying the Database Safely
    have been satisfied.
  • Any new migrations/schema changes:
    • Follow our guidelines for zero-downtime deploys (see Zero-Downtime Deploys)
    • Have been communicated to #dp3-engineering
    • Secure migrations have been tested using scripts/run-prod-migrations
  • There are no aXe warnings for UI.
  • This works in Supported Browsers and their phone views (Chrome, Firefox, IE, Edge).
  • Any new client dependencies (Google Analytics, hosted libraries, CDNs, etc) have been:
  • 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 Pivotal acceptance criteria been met for this change?

References

None

Screenshots

None

@codecov

This comment has been minimized.

Copy link

commented May 8, 2019

Codecov Report

Merging #2105 into master will increase coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2105      +/-   ##
==========================================
+ Coverage   60.26%   60.29%   +0.03%     
==========================================
  Files         229      230       +1     
  Lines       13331    13341      +10     
==========================================
+ Hits         8033     8043      +10     
  Misses       4326     4326              
  Partials      972      972
1 similar comment
@codecov

This comment has been minimized.

Copy link

commented May 8, 2019

Codecov Report

Merging #2105 into master will increase coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2105      +/-   ##
==========================================
+ Coverage   60.26%   60.29%   +0.03%     
==========================================
  Files         229      230       +1     
  Lines       13331    13341      +10     
==========================================
+ Hits         8033     8043      +10     
  Misses       4326     4326              
  Partials      972      972
parts := strings.SplitN(arg, "=", 2)
if len(parts) == 2 {
filters[parts[0]] = parts[1]
}

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj May 8, 2019

Contributor

Should we print a warning if the args aren't formatted right?

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj May 8, 2019

Contributor

I might be misunderstanding how this works, but are args all the command line flags and the list of arguments? For example, are the args for ecs-show-service-logs --environment staging filter1=value filter2=value equivalent to --environment=staging filter1=value filter2=value or filter1=value filter2=value ? Because I was worried --environment=staging would get parsed here.

This comment has been minimized.

Copy link
@pjdufour-truss

pjdufour-truss May 9, 2019

Author Contributor

Args should include all non-flags through the power of cobra (os.Args is different)!

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj May 9, 2019

Contributor

Awesome!

@chrisgilmerproj
Copy link
Contributor

left a comment

🚀

@pjdufour-truss pjdufour-truss merged commit b439157 into master May 9, 2019

20 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
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_tasks Your tests passed on CircleCI!
Details
ci/circleci: build_tools Your tests passed on CircleCI!
Details
ci/circleci: client_test Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_api Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_mymove Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_office Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_tsp 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
ci/circleci: server_test_coverage Your tests passed on CircleCI!
Details
codecov/patch Coverage not affected when comparing 6d584fa...fdcfdc0
Details
codecov/project/go 60.12% (+0.03%) compared to 6d584fa
Details

@pjdufour-truss pjdufour-truss deleted the ecs_service_logs_filters branch May 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.