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

Re-introduce the instruments middleware #312

Merged
merged 1 commit into from
May 30, 2017
Merged

Conversation

gudmundur
Copy link
Member

With the introduction of canonical log lines, we dropped the Pliny::Middleware::Instruments as a default middleware, see here. The side effect of that is that the start of a request is not logged. The default log lines emitted for a request are:

app=hello deployment=development request_id=c3ae2fae-0b29-4d27-9f05-402fcce7a95e count#hello.requests=1
app=hello deployment=development canonical_log_line request_id=c3ae2fae-0b29-4d27-9f05-402fcce7a95e request_ip=127.0.0.1 request_method=GET request_path=/ request_user_agent=curl/7.51.0 request_route_signature=/ response_length=6 response_status=200 timing_total_elapsed=0.033
app=hello deployment=development request_id=c3ae2fae-0b29-4d27-9f05-402fcce7a95e measure#hello.requests.latency=0.033
app=hello deployment=development request_id=c3ae2fae-0b29-4d27-9f05-402fcce7a95e count#hello.requests.status.2xx=1
app=hello deployment=development request_id=bdd42e63-005e-48b4-9d70-99698288c0e6 count#hello.requests=1

The initial count#hello.requests=1 happens at the beginning of a request but is a metrics line, and will be dropped if a different metric backend than logfmt is used.

This change brings back the Instruments middleware, although much of the information reported by it is already available in the canonical log line. It does have the benefit of indicating the start and end of a request.

@gudmundur
Copy link
Member Author

@brandur What do you think about this one? Do you think we should change the Instruments middleware or just leave it as is?

@brandur
Copy link
Member

brandur commented May 29, 2017

I don't have too strong of an opinion on this one. Looking at my canonical log line patch, if I were to do this all over again, I probably would have just left the instruments middleware in as kind of a principle of least surprise thing, but I think it could go either way.

Another option might be to add in some other logger that prints out some basic information about the requests (maybe HTTP headers and the like), but that's probably overdesigning for now.

In short, if having instruments around is something that seems like it would be beneficial to you, then +1.

@gudmundur gudmundur merged commit 83f011f into master May 30, 2017
@gudmundur gudmundur deleted the bring-back-instruments branch May 30, 2017 07:21
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