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

Apollo Engine Reporting Duration Histogram #3671

Merged
merged 12 commits into from
Feb 12, 2020

Conversation

jsegaran
Copy link
Contributor

This PR creates a duration histogram class that can be used during reporting to create summary statistics client-side instead of sending all the traces to the server. This will allow a smaller number of traces to be reported to Apollo.

@apollo-cla
Copy link

@jsegaran: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

@jsegaran jsegaran requested a review from glasser January 12, 2020 06:59
.gitignore Outdated
@@ -23,3 +23,6 @@ node_modules/

# Mac OS
.DS_Store

# Idea files
Copy link
Member

Choose a reason for hiding this comment

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

I might say InteliiJ here, which is a more recognizable/searchable name

export class DurationHistogram {
private buckets: number[];

public serialize(): number[] {
Copy link
Member

Choose a reason for hiding this comment

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

I'd like this to be called toArray so that it works with protobufjs/protobuf.js#1302 (which our protobufjs fork includes: #3530)


public serialize(): number[] {
let bufferedZeroes = 0;
let outputArray = Array<number>();
Copy link
Member

Choose a reason for hiding this comment

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

This can be const instead of let, because you never assign to outputArray again. I also think it's probably more idiomatic to write this as:

const outputArray: number[] = [];

if (buckets) {
this.buckets = Object.assign([], buckets);
} else {
this.buckets = Array<number>(383).fill(0);
Copy link
Member

Choose a reason for hiding this comment

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

Looks like we use 384 in our backend?


constructor(buckets?: number[]) {
if (buckets) {
this.buckets = Object.assign([], buckets);
Copy link
Member

Choose a reason for hiding this comment

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

I'm not super sure how Object.assign works on an array. You can copy an array with [...buckets]. That won't extend it to 384 — not sure the goal for this argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed it. I wanted a way to go from buckets to DurationHistogram object.


public combine(otherHistogram: DurationHistogram) {
for (let i = 0; i < otherHistogram.buckets.length; i++) {
this.buckets[i] += otherHistogram.buckets[i]
Copy link
Member

Choose a reason for hiding this comment

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

I note no semicolon here. While apollo-server as a whole does not use any automated formatting tools, I'd recommend using prettier inside AER (which currently should be still formatted with prettier) unless you really enjoy thinking carefully about formatting.

return outputArray;
}

public increment(bucket: number) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe call this incrementBucket, and make sure to provide a function that maps from duration to bucket number?

Copy link
Member

Choose a reason for hiding this comment

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

Note btw that (see comment on durationHrTimeToNanos) AER assumes that durations are all less than 104 days and passes durations around as nano numbers.

@@ -0,0 +1,32 @@
import {DurationHistogram} from "../durationHistogram";

describe("Duration histogram tests", () => {
Copy link
Member

Choose a reason for hiding this comment

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

I think there are a lot more DurationHistogramTest.kt tests that could be ported (eg all of testSerialize)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added most of the ones I felt were relevant. I left out the builder tests, and the de-serialization tests.

@jsegaran jsegaran requested a review from glasser February 2, 2020 02:32
@jsegaran jsegaran force-pushed the jsegaran/2020/1/duration_histogram branch from 594654a to 81f2ca3 Compare February 2, 2020 04:54
Copy link
Member

@glasser glasser left a comment

Choose a reason for hiding this comment

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

A few minor suggestions. If you don't want to start with smaller bucket lists then this is mergeable now, but I do think that would be nice.

const log = Math.log(durationNs / 1000.0);
const unboundedBucket = Math.ceil(log / DurationHistogram.EXPONENT_LOG);

// Comapre <= 0 to catch -0
Copy link
Member

Choose a reason for hiding this comment

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

Also this catches -Infinity (which is what we get when durationNs = 0). Probably worth noting that. Also typo on "compare"

}

constructor(buckets?: number[]) {
this.buckets = Array<number>(DurationHistogram.BUCKET_COUNT).fill(0);
Copy link
Member

Choose a reason for hiding this comment

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

Most of the buckets are generally empty (eg, 1 minute is only bucket 188 so half of the buckets are only used for durations over a minute). It might be a better use of memory to grow the bucket array lazily, starting with the list either empty or some reasonable smaller size.

To grow the array from length L to M you can do this.buckets.length = M; this.buckets.fill(0, L) I think. incrementBucket and combine would both have to ensure there are enough buckets.

Maybe this is a fancy thing we could do later? But I think it's straightforward enough and would save a fair amount of memory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. I'm worried somewhat about perf, so I think this should be an adjustable value. I'm also wondering if the arrays get fairly sparse we could use a half-compacted format, but that would be a latter follow up.

});

it("testToArray", () => {
assertToArrayHelper([], []);
Copy link
Member

Choose a reason for hiding this comment

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

You can define the helper right here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks didn't know that.

@jsegaran jsegaran requested a review from glasser February 4, 2020 19:25

// Extend the array if we haven't gotten it long enough to handle the new bucket
if (bucket >= this.buckets.length) {
let tempBufferLength = bucket - this.buckets.length + 1;
Copy link
Member

Choose a reason for hiding this comment

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

I think you can do this.buckets.length = bucket + 1; this.buckets.fill(0, oldLength) instead of making a temporary array and then pushing it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didnt' know that was possible. Fixed it.

this.buckets.push(...emptyBucketBuffer);
}

this.buckets[bucket]+=value;
Copy link
Member

Choose a reason for hiding this comment

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

looks like you haven't prettiered lately

constructor(buckets?: number[]) {
this.buckets = Array<number>(DurationHistogram.BUCKET_COUNT).fill(0);
constructor(initSize = 74, buckets?: number[]) {
let arrayInitSize = Math.max(buckets?.length || 0, initSize);
Copy link
Member

Choose a reason for hiding this comment

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

use const, not let

?. in TS, a brave new world! How exciting.

Does this constructor let you pass buckets but no initSize? Maybe the parameters should be in the other order?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't I felt like we were more likely to want to pass in the initSize than buckets so I thought this would work better. I don't see us passing buckets that often while doing reporting?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe this should just be an options object instead of multiple optional parameters.

@jsegaran jsegaran requested a review from glasser February 5, 2020 22:23
this.buckets[bucket] += value;
}

public combine(otherHistogram: DurationHistogram) {
Copy link
Member

Choose a reason for hiding this comment

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

This should call incrementBucket so that it does the expansion stuff.

@@ -0,0 +1,81 @@
export interface DurationHistogramOptions {
initSize?: number
Copy link
Member

Choose a reason for hiding this comment

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

looks un-prettier

@jsegaran jsegaran requested a review from glasser February 7, 2020 06:01
@jsegaran jsegaran force-pushed the jsegaran/2020/1/duration_histogram branch from f53e4ff to e63a74d Compare February 7, 2020 06:04
@jsegaran jsegaran merged commit acdadbc into master Feb 12, 2020
@jsegaran jsegaran deleted the jsegaran/2020/1/duration_histogram branch February 12, 2020 01:08
@@ -10,6 +10,7 @@ The version headers in this history reflect the versions of Apollo Server itself
> The changes noted within this `vNEXT` section have not been released yet. New PRs and commits which introduce changes should include an entry in this `vNEXT` section as part of their development. When a release is being prepared, a new header will be (manually) created below and the the appropriate changes within that release will be moved into the new section.

- `apollo-server-core`: Update GraphQL Playground to latest version to remove a rogue curly-brace appearing in the top-right corner of the interface under certain conditions. [PR #3702](https://github.com/apollographql/apollo-server/pull/3702) [Playground PR](https://github.com/apollographql/graphql-playground/pull/21)
- `apollo-engine-reporting`: Added duration histogram methods
Copy link
Member

Choose a reason for hiding this comment

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

I don't actually know that this is necessary, since there's no user-visible effect yet. We can add a CHANGELOG once the code is not dead.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants