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.

Sign up for GitHub

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

Jump to bottom

rgw: add option to log custom HTTP headers (rgw_log_http_headers) #7639

Merged
merged 3 commits into from Dec 14, 2016
Merged

rgw: add option to log custom HTTP headers (rgw_log_http_headers) #7639

merged 3 commits into from Dec 14, 2016

Conversation

mattbenjamin
Copy link
Contributor

Tracks headers that should be handled conditionally (currently,
can only log, so using minimal structure to represent the
mapping).

Signed-off-by: Matt Benjamin mbenjamin@redhat.com

@mattbenjamin
Copy link
Contributor Author

I don't quite understand how to tie them together. IIUC, Apache's builtin logging won't capture arbitrary headers, though there are add-in modules that let you do that, of course?

@yehudasa
Copy link
Member

@mattbenjamin it's either built in, or through mod_log_config (or other module). We can do something within civetweb (which I'm not sure we want). Or maybe we can add the extra headers to the access log message that we dump (through the civetweb access log callback)

@mattbenjamin mattbenjamin force-pushed the wip-rgw-header branch 3 times, most recently from 0804373 to e3468fe Compare November 14, 2016 23:21
@mattbenjamin
Copy link
Contributor Author

@yehudasa rebased

@mattbenjamin mattbenjamin changed the title add rgw_log_http_headers option - DNM rgw: add option to log custom HTTP heades (rgw_log_http_headers) Nov 15, 2016
@mattbenjamin
Copy link
Contributor Author

@yehudasa updated

@mattbenjamin
Copy link
Contributor Author

@yehudasa oops, repushed

yehudasa
yehudasa reviewed Nov 17, 2016
View reviewed changes
src/rgw/rgw_log.h

void encode(bufferlist &bl) const {
ENCODE_START(8, 5, bl);
ENCODE_START(9, 5, bl); // was 8
Copy link
Member

Choose a reason for hiding this comment

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

@mattbenjamin remove this comment

yehudasa
yehudasa reviewed Nov 17, 2016
View reviewed changes
src/rgw/rgw_log.h
if (struct_v >= 8) {
::decode(object_owner, p);
::decode(bucket_owner, p);
if (struct_v >= 9) {
Copy link
Member

Choose a reason for hiding this comment

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

@mattbenjamin for code readability I'd prefer to avoid nested ifs like this, and with regular compile optimization it shouldn't affect execution time

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yehudasa ok, I find this more readable, but I don't mind changing it

yehudasa
yehudasa reviewed Nov 17, 2016
View reviewed changes
src/rgw/rgw_log.h
@@ -37,7 +37,7 @@ struct rgw_log_entry {
headers_map x_headers;

void encode(bufferlist &bl) const {
ENCODE_START(9, 5, bl); // was 8
ENCODE_START(9, 5, bl);
Copy link
Member

Choose a reason for hiding this comment

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

@mattbenjamin ah, this was already done

@mattbenjamin
Copy link
Contributor Author

mattbenjamin commented Nov 17, 2016 •
edited

(mix up, ignore deleted)

@ghost
Copy link

ghost commented Nov 22, 2016

jenkins test this please (no logs)

@mattbenjamin mattbenjamin changed the title rgw: add option to log custom HTTP heades (rgw_log_http_headers) rgw: add option to log custom HTTP headers (rgw_log_http_headers) Dec 8, 2016
@mattbenjamin
Copy link
Contributor Author

mattbenjamin commented Dec 8, 2016 •
edited

@yehudasa your most recent comments were deleted apparently by github; could you re-write the list of current blockers for this change? tia, Matt

IIRC, one of the changes was to update docs. Pushed part of that.

@mattbenjamin
Copy link
Contributor Author

@yehudasa ARG. Will rebase by hand.

@mattbenjamin
rgw: add rgw_log_http_headers option
b82919a 
Tracks headers that should be handled conditionally (currently,
can only log, so using minimal structure to represent the
mapping).

Adds map of custom headers to rgw_log_entry, and populate it with
headers pre-selected for custom logging in RGWREST.  Added to encoder
and Formatter output.

Some additional optimization possible.

Signed-off-by: Matt Benjamin <mbenjamin@redhat.com>
@mattbenjamin
rgw: custom headers: use boost::container::flat_map
4092c97 
Use this occasion of serialized map to implement encoders
for boost::container::flat_map, which is optimized for
in-order insertion.

After some discussion, I'm proposing to just add the new template
forms, rather than (e.g.) adding either a large amount of
specialization machinery, or alternatively using C-style
macros.

Signed-off-by: Matt Benjamin <mbenjamin@redhat.com>
@mattbenjamin
Copy link
Contributor Author

@yehudasa updated--I've verified this by hand, details below:

ops log

rgw enable ops log = true
rgw ops log socket path = /tmp/opslog
rgw log http headers = "http_x_forwarded_for, http_expect, http_content_md5"

tested with

sudo ncat -U /tmp/opslog

prints:

{"bucket":"bu_4","time":"2016-12-13 22:45:36.073961Z","time_local":"2016-12-13 17:45:36.073961","remote_addr":"","user":"testuser","operation":"PUT","uri":"/bu_4/bu_4_o32","http_status":"200","error_code":"","bytes_sent":0,"bytes_received":0,"object_size":21,"total_time":7699,"user_agent":"Boto/2.40.0 Python/2.7.11 Linux/4.6.6-200.fc23.x86_64","referrer":"","http_x_headers":[{"HTTP_CONTENT_MD5":"7FYUagKWrRgUAxkBdLZ80g=="},{"HTTP_EXPECT":"100-Continue"}]},

yehudasa
yehudasa approved these changes Dec 13, 2016
View reviewed changes
Copy link
Member

@yehudasa yehudasa left a comment

Choose a reason for hiding this comment

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

@mattbenjamin lgtm. For some reason the signed-off-by test failed

@mattbenjamin
Copy link
Contributor Author

@yehudasa huh, will update

@mattbenjamin
rgw: add 'rgw log http headers' gloss to config-ref.rst
04bf0f8 
Explain the option for upstream doc.

Signed-off-by: Matt Benjamin <mbenjamin@redhat.com>
@mattbenjamin
Copy link
Contributor Author

@yehudasa actually sign-off was present on the one marked bad, but I did notice that there was only the heading line and sign-off; maybe a parsing error?

@yehudasa
Copy link
Member

@mattbenjamin probably

@yehudasa yehudasa merged commit 9b331f9 into ceph:master Dec 14, 2016
This was referenced Dec 14, 2016
Merged
Closed
mattbenjamin added a commit to linuxbox2/ceph that referenced this pull request Jan 12, 2017
@mattbenjamin
use std::map
9fd29b4 
(adaptation commit for ceph#7639)

Signed-off-by: Matt Benjamin <mbenjamin@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yehudasa yehudasa yehudasa approved these changes

Assignees

@yehudasa yehudasa

Labels
feature rgw
Projects
None yet
Milestone
No milestone
3 participants
@mattbenjamin @yehudasa @oritwas