Skip to content

Support Mixer HTTP filter to report sent.bytes and received.bytes attributes#1372

Merged
istio-merge-robot merged 3 commits intoistio:masterfrom
JimmyCYJ:add-attribute-sent-bytes-received-bytes
Apr 16, 2018
Merged

Support Mixer HTTP filter to report sent.bytes and received.bytes attributes#1372
istio-merge-robot merged 3 commits intoistio:masterfrom
JimmyCYJ:add-attribute-sent-bytes-received-bytes

Conversation

@JimmyCYJ
Copy link
Copy Markdown
Member

@JimmyCYJ JimmyCYJ commented Apr 8, 2018

What this PR does / why we need it:Support Mixer HTTP filter to send attributes "request.total_size" and "response.total_size" in Report() calls. "response.total_size" records total response sent in bytes, including response headers, body, and trailers. "request.total_size" records total request received in bytes, including request headers, body, and trailers.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #1370

Special notes for your reviewer:

Release note:

NONE

@istio-testing istio-testing requested review from geeknoid and lizan April 8, 2018 05:10
@googlebot googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Apr 8, 2018
@JimmyCYJ JimmyCYJ requested review from qiwzhang and removed request for geeknoid and lizan April 8, 2018 05:10
const char AttributeName::kRequestReferer[] = "request.referer";
const char AttributeName::kRequestScheme[] = "request.scheme";
const char AttributeName::kRequestSize[] = "request.size";
const char AttributeName::kReceivedBytes[] = "received.bytes";
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi @douglas-reid I felt it is better to name these two new attributes as "request.total_size" and "response.total_size" so it consists with existing attributes of "request.size"

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@qiwzhang request.total_size and response.total_size sgtm.

@qiwzhang
Copy link
Copy Markdown
Contributor

qiwzhang commented Apr 9, 2018

Please also update this file https://github.com/istio/api/blob/master/mixer/v1/global_dictionary.yaml for the new attributes you added

@qiwzhang
Copy link
Copy Markdown
Contributor

qiwzhang commented Apr 9, 2018

@qiwzhang
Copy link
Copy Markdown
Contributor

qiwzhang commented Apr 9, 2018

Please make sure to change Pilot code for the filter type change.

@JimmyCYJ JimmyCYJ requested a review from mandarjog April 10, 2018 01:23
@JimmyCYJ JimmyCYJ force-pushed the add-attribute-sent-bytes-received-bytes branch from 9f71a81 to e55fde3 Compare April 15, 2018 04:03
@qiwzhang
Copy link
Copy Markdown
Contributor

/lgtm
/approve

@istio-testing
Copy link
Copy Markdown
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qiwzhang

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@istio-merge-robot
Copy link
Copy Markdown

/test all [submit-queue is verifying that this PR is safe to merge]

@istio-merge-robot
Copy link
Copy Markdown

Automatic merge from submit-queue.

@istio-merge-robot istio-merge-robot merged commit d20cd6d into istio:master Apr 16, 2018
@JimmyCYJ JimmyCYJ deleted the add-attribute-sent-bytes-received-bytes branch April 16, 2018 17:31
const char AttributeName::kResponseHeaders[] = "response.headers";
const char AttributeName::kResponseSize[] = "response.size";
const char AttributeName::kResponseBodySize[] = "response.size";
const char AttributeName::kResponseTotleSize[] = "response.totle_size";
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

sorry to be late with this comment, but should these attributes be request.total_size and response.total_size ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

request.total_size is added at line 32. Prior to this PR, we already let Mixer filter to report request.size and response.size, and they are actually request body size and response body size. So I update the variable name (e.g. s/kResponseSize/kResponseBodySize) to make it less confusing.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@JimmyCYJ I'm actually wondering about the difference between totle and total (and what the name of the attribute is).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That is typo. I will fix it soon. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support Mixer HTTP filter to send attributes 'sent_bytes' and 'received_bytes'.

6 participants