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

chore: Clarify bytes metrics in component spec #12912

Merged
merged 17 commits into from
Aug 11, 2022
Merged

Conversation

binarylogic
Copy link
Contributor

@binarylogic binarylogic commented May 31, 2022

This PR updates the requirements for the ComponentBytesReceived and ComponentBytesSent events:

  1. Renamed ComponentBytesReceived to SourceNetworkBytesReceived.
  2. Renamed ComponentBytesSent to SinkNetworkBytesSent.
  3. Clarified that the network bytes are best effort based on the source/sink capability. In other words, it should be the closest representation possible to the raw network bytes based on client limitations.
  4. Clarified that the ComponentEventsSent and ComponentEventsReceived should emit a byte_size that reflects an estimated size for JSON encoding.

Signed-off-by: Ben Johnson <bjohnson@binarylogic.com>
Signed-off-by: Ben Johnson <bjohnson@binarylogic.com>
@binarylogic binarylogic requested review from tobz and jszwedko May 31, 2022 16:27
@netlify
Copy link

netlify bot commented May 31, 2022

Deploy Preview for vector-project ready!

Name Link
🔨 Latest commit 2b62c7e
🔍 Latest deploy log https://app.netlify.com/sites/vector-project/deploys/62f40c63e2f1260009f369cc
😎 Deploy Preview https://deploy-preview-12912--vector-project.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Member

@jszwedko jszwedko left a comment

Choose a reason for hiding this comment

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

Agreed with publishing a separate metric for enterprise billing though I can see what you are saying about decoupling it in-case the billing strategy changes. I think ComponentAcceptedBytes is reasonable.

Do you mean for the ComponentBytesReceived and ComponentBytesSent events to indicate raw I/O now? I think there are some discrepancies below, if so. For example, pull-based sources will emit ComponentBytesSent for raw output for any requests they make. The aws_ec2_metadata transform will also emit raw I/O metrics. I might suggest separating out this change from the billing change to organize discussion around each.

Also, should we start keeping a changelog on the spec with this change? I think that'll make it easier to talk about the state of implementation of the spec.

docs/specs/component.md Outdated Show resolved Hide resolved
docs/specs/component.md Outdated Show resolved Hide resolved
Signed-off-by: Ben Johnson <bjohnson@binarylogic.com>
Signed-off-by: Ben Johnson <bjohnson@binarylogic.com>
Signed-off-by: Ben Johnson <bjohnson@binarylogic.com>
@binarylogic binarylogic changed the title chore: Update component spec with explicit billing chore: Clarify bytes metrics in component spec Jun 8, 2022
Signed-off-by: Ben Johnson <bjohnson@binarylogic.com>
Signed-off-by: Ben Johnson <bjohnson@binarylogic.com>
@binarylogic
Copy link
Contributor Author

Talked with @jszwedko and simplified the requirements. I updated the description to summarize the changes.

Signed-off-by: Ben Johnson <bjohnson@binarylogic.com>
Copy link
Contributor

@tobz tobz left a comment

Choose a reason for hiding this comment

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

I'm happy with a lot of the simplification happening here, but left some comments/requests on some of the language which feels a bit too ambiguous/hard to grok.

docs/specs/component.md Outdated Show resolved Hide resolved
docs/specs/component.md Outdated Show resolved Hide resolved
docs/specs/component.md Outdated Show resolved Hide resolved
docs/specs/component.md Outdated Show resolved Hide resolved
Signed-off-by: Ben Johnson <bjohnson@binarylogic.com>
Signed-off-by: Ben Johnson <bjohnson@binarylogic.com>
@binarylogic
Copy link
Contributor Author

Ok, I pushed a big update that I think really clarifies the different between event bytes and network bytes. Here's what I changed:

  1. Renamed ComponentBytesReceived to SourceNetworkBytesReceived.
  2. Renamed ComponentBytesSent to SinkNetworkBytesSent.
  3. Removed the framing nuance entirely. No framed tag, etc.
  4. Clarified that the network bytes are best effort based on the source/sink capability. In other words, it should be the closest representation possible to the raw network bytes based on client limitations.
  5. Clarified that the ComponentEventsSent and ComponentEventsReceived should emit a byte_size that reflects an estimated size for JSON encoding.

docs/specs/component.md Outdated Show resolved Hide resolved
Copy link
Member

@jszwedko jszwedko left a comment

Choose a reason for hiding this comment

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

👍 to these changes in general. I left some comments below inline.

For the billing metric, is my reading correct that we would now use component_received_event_bytes_total on sources?

docs/specs/component.md Outdated Show resolved Hide resolved
docs/specs/component.md Outdated Show resolved Hide resolved
docs/specs/component.md Outdated Show resolved Hide resolved
docs/specs/component.md Outdated Show resolved Hide resolved
docs/specs/component.md Outdated Show resolved Hide resolved
docs/specs/component.md Outdated Show resolved Hide resolved
Signed-off-by: Ben Johnson <bjohnson@binarylogic.com>
@binarylogic
Copy link
Contributor Author

For the billing metric, is my reading correct that we would now use component_received_event_bytes_total on sources?

Assuming this is an accurate representation of the JSON encoding, I don't see why not. My only concern is enriching within the source and how customers would feel about that. Regardless, I don't want to encourage customers to use an inferior protocol because it's cheaper. We should try to normalize the bytes we're charging post-process.

Copy link
Member

@jszwedko jszwedko left a comment

Choose a reason for hiding this comment

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

Looks good to me! I do think we should start tracking revisions to this document as a changelog in the doc itself to be able to easily refer to versions of the doc, but we can start that with any future ones.

docs/specs/component.md Outdated Show resolved Hide resolved
Co-authored-by: Jesse Szwedko <jesse@szwedko.me>
Copy link
Contributor

@tobz tobz left a comment

Choose a reason for hiding this comment

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

Overall, this looks good and makes sense to me.

I left some suggestions for improving clarity, but they're non-blocking.

docs/specs/component.md Outdated Show resolved Hide resolved
docs/specs/component.md Outdated Show resolved Hide resolved
docs/specs/component.md Outdated Show resolved Hide resolved
docs/specs/component.md Outdated Show resolved Hide resolved
binarylogic and others added 5 commits August 10, 2022 15:48
Co-authored-by: Toby Lawrence <tobz@users.noreply.github.com>
Co-authored-by: Toby Lawrence <tobz@users.noreply.github.com>
Co-authored-by: Toby Lawrence <tobz@users.noreply.github.com>
Co-authored-by: Toby Lawrence <tobz@users.noreply.github.com>
Signed-off-by: Ben Johnson <bjohnson@binarylogic.com>
Copy link
Contributor

@JeanMertz JeanMertz left a comment

Choose a reason for hiding this comment

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

All of this makes sense to me 👍

@binarylogic binarylogic merged commit 148a409 into master Aug 11, 2022
@binarylogic binarylogic deleted the billing-metric branch August 11, 2022 12:45
jdrouet pushed a commit that referenced this pull request Aug 22, 2022
* Clarify ComponentBytesReceived to differentiate billing

Signed-off-by: Ben Johnson <bjohnson@binarylogic.com>

* Add ComponentBilled event

Signed-off-by: Ben Johnson <bjohnson@binarylogic.com>

* Remove explicit billing metric based on recent discussions with rev eng

Signed-off-by: Ben Johnson <bjohnson@binarylogic.com>

* Clarify language

Signed-off-by: Ben Johnson <bjohnson@binarylogic.com>

* Fix line lengths

Signed-off-by: Ben Johnson <bjohnson@binarylogic.com>

* Simplify bytes events requirements

Signed-off-by: Ben Johnson <bjohnson@binarylogic.com>

* Add url tag

Signed-off-by: Ben Johnson <bjohnson@binarylogic.com>

* Add optional s qualifier

Signed-off-by: Ben Johnson <bjohnson@binarylogic.com>

* Clarify source/sink naming

Signed-off-by: Ben Johnson <bjohnson@binarylogic.com>

* Clarify wording around network and event bytes

Signed-off-by: Ben Johnson <bjohnson@binarylogic.com>

* Address Jesses feedback

Signed-off-by: Ben Johnson <bjohnson@binarylogic.com>

* Update docs/specs/component.md

Co-authored-by: Jesse Szwedko <jesse@szwedko.me>

* Update docs/specs/component.md

Co-authored-by: Toby Lawrence <tobz@users.noreply.github.com>

* Update docs/specs/component.md

Co-authored-by: Toby Lawrence <tobz@users.noreply.github.com>

* Update docs/specs/component.md

Co-authored-by: Toby Lawrence <tobz@users.noreply.github.com>

* Update docs/specs/component.md

Co-authored-by: Toby Lawrence <tobz@users.noreply.github.com>

Signed-off-by: Ben Johnson <bjohnson@binarylogic.com>
Co-authored-by: Jesse Szwedko <jesse@szwedko.me>
Co-authored-by: Toby Lawrence <tobz@users.noreply.github.com>
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.

None yet

5 participants