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

Parse GraphQL responses for accurate success rates #636

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

parkerholladay
Copy link

Background

Because GraphQL generally returns 200 even when there are errors (unless something goes horribly wrong), attacking GraphQL endpoints can give false positives on success rate. These changes will inspect the http response body for GraphQL errors and tally success/errors accordingly.

Checklist

  • Git commit messages conform to community standards.
  • Each Git commit represents meaningful milestones or atomic units of work.
  • Changed or added code is covered by appropriate tests.

Copy link
Owner

@tsenart tsenart left a comment

Choose a reason for hiding this comment

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

Hey, thank you for the contribution. I think starting do be application protocol aware on top of HTTP is a big can of complexity, since there's an endless number of them, not just GraphQL.

You can emulate this same behavior without this code change by writing a program that reads Vegeta results, decodes response bodies in the same way that you are doing here, and adjusts the response code / error field of the result in accordance.

Then just write that out to stdout using Vegeta.Encoder and every other Vegeta sub command will work with it.

We could add a -protocol flag to the Vegeta encode sub command, and if it's present, we do that additional work of protocol specific re-encoding there.

But I don't want us to pay that cost in attack.

@parkerholladay
Copy link
Author

Thanks for the quick feedback @tsenart and @peterbourgon. I hear what you're saying, so it's good to have you talk through it. We've created a program that does some of what you're describing, but it's pretty bespoke for the use case we have right now. I'd like it to make it work for others and keep with the model you have of piping commands together.

I'll take a stab at what you've suggested and re-submit. We're loving vegeta and I'm glad you've rediscovered your excitement for it.

@parkerholladay parkerholladay changed the title Add flag to inspect response for graphql errors WIP: GraphQL awareness for accurate success rates Jul 25, 2023
@parkerholladay parkerholladay changed the title WIP: GraphQL awareness for accurate success rates Parse GraphQL responses for accurate success rates Jul 26, 2023
Copy link
Author

@parkerholladay parkerholladay left a comment

Choose a reason for hiding this comment

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

I've updated the PR based on your feedback @tsenart. In working through the changes, it seemed to make sense to keep the -protocol flag on the attack command rather than moving it to encode. This way, the encode that happens, after the attack run is processed, will have the protocol. Consequently, you won't be required to pipe to encode before you can run reports or whatever else.

If you still don't like having that flag on attack let me know.

lib/results.go Outdated
dec.FieldsPerRecord = 12
func NewCSVDecoder(rd io.Reader) Decoder {
dec := csv.NewReader(rd)
dec.FieldsPerRecord = 13
Copy link
Author

@parkerholladay parkerholladay Jul 26, 2023

Choose a reason for hiding this comment

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

This FieldsPerRecord is one change I am not 100% sure about. If someone has a result encoded to csv saved to a file by an older version and tries to re-encode it, new versions will not be able to read it.

How do you feel about the options between a) not setting FieldsPerRecord, or b) keeping it at 13 and dealing with the fallout?

@@ -276,27 +291,70 @@ type jsonResult Result
// NewJSONEncoder returns an Encoder that dumps the given *Results as a JSON
// object.
func NewJSONEncoder(w io.Writer) Encoder {
var jw jwriter.Writer
var enc jwriter.Writer
Copy link
Author

Choose a reason for hiding this comment

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

Rename for consistency with other decoder/encoder functions

func NewJSONDecoder(r io.Reader) Decoder {
rd := bufio.NewReader(r)
func NewJSONDecoder(rd io.Reader) Decoder {
dec := bufio.NewReader(rd)
Copy link
Author

Choose a reason for hiding this comment

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

Rename for consistency with other decoder/encoder functions

Copy link
Owner

@tsenart tsenart left a comment

Choose a reason for hiding this comment

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

@parkerholladay: Thanks for making progress on this. I still don't think it's worth it to make attack protocol aware, but think -protocol flag in encode makes sense. It seems like you're hesitant about some cost of piping results to a sub command. Can you elaborate on that? In any case, happy to review a patch to the encode command.

@peterbourgon
Copy link
Contributor

peterbourgon commented Jul 30, 2023

I agree with @tsenart. I'm coming at it from a slightly different angle.

This PR would change the Result type to no longer be an HTTP result, but actually an arbitrary protocol result. It would allow the specific value of the Protocol field to influence the semantics of other fields like Error. That's a pretty big change! It means vegeta attack isn't an HTTP load testing tool any more, it's an arbitrary protocol load testing tool. Attack results can't be interpreted on their own, they require a priori knowledge of the rules for the individual protocols. That has unknowable downstream consequences. A program today may (reasonably!) treat a Result with Code 200 as a success; after this change, that would no longer be valid. Changes like this PR makes to the Metrics type (which has this behavior) are a little fragile to stand the test of time.

I'm not saying it's necessarily a bad idea, but I do think expanding the scope of Vegeta in this way requires a lot more careful consideration and planning. Until then, I think it makes a lot of sense to do per-protocol result interpretation at the encode stage. It would have far more narrowly scoped impact, and I'm sure it can be just as efficient, or even more efficient, than doing it during the attack stage. Happy to help with that, if there are concerns!

edit: Ah, I realize I may be describing something that goes even a bit further than what @tsenart is requesting. Maybe treat this comment with a grain of salt :)

@parkerholladay
Copy link
Author

It seems like you're hesitant about some cost of piping results to a sub command. Can you elaborate on that? In any case, happy to review a patch to the encode command.

@tsenart, I have no reservations about piping to the encode sub-command, but to get metrics to calculate correctly, each result needs to be tagged in some way so we can distinguish between a basic http 200 result with errors (not typical) and a gql 200 result with errors. The easiest way to do that was to tag the result in the attack, then encode deals with all the actual transformations, but it could be tagged similarly by encode.

So, let me see if I understand you correctly, are you saying that you just want me to move the -protocol argument from attack to encode and you're good with the rest of the changes as-is?

@tsenart
Copy link
Owner

tsenart commented Aug 2, 2023

What I have in mind is that attack doesn't change whatsoever. All the logic of re-interpreting the response body and errors according to a different protocol happens in encode.

@parkerholladay
Copy link
Author

Again, I want to be clear, moving -protocol to the encode command will change the signature of the encode function across the app to look something like:

func (enc Encoder) Encode(r *Result, protocol string) error {
    ...
}

And, existing usages in attack will have to be passed an empty string like so enc.Encode(r, ""). This could cause breaking changes for those who use vegeta as a library.

Lastly, for success metrics to be accurately calculated, the Result type does need to know if it was encoded as gql or http. If you have other ideas of how the metrics.success can be calculated without the protocol being adding to Result, I'm happy to try implementing it some other way.

@peterbourgon
Copy link
Contributor

peterbourgon commented Aug 3, 2023

The Result type has always represented a direct/raw HTTP response. This PR would allow the Result type to represent either an HTTP response or a GQL response, based on a protocol string. More specifically, it would allow a protocol string to transform an HTTP-class Result into a GQL-class Result. And the discussion so far is about where to apply that transformation: in the attack phase, or in the encode phase.

I'd propose an alternative approach. Don't modify the Result type at all: let it continue to represent an HTTP result.

Instead, add a new e.g. GQLResult type on top of the Result type, which would implement the GQL-specific changes, e.g. determining success based on a specific parsing of the response body rather than the status code by itself. Then, refactor the code which consumes Results to not take a concrete Results type, but instead to take an abstraction, which can be satisfied by both Results and GQLResults – as well as any other protocol-specific Results types in the future. That abstraction would define operations, like Success, which can be calculated differently for each underlying type.

type HitResult interface {
    Success() bool
    ...
}

func (r *Result) Success() bool { 
    return r.StatusCode within 100..299
}

type GQLResult struct {
    Result
}

func (r *GQLResult) Success() bool {
    return GQL-specific parsing of r.Result.Body
}

func (m *Metrics) Observe(r HitResult) {
    ...
    if r.Success() {
        m.markSuccess(...)
    }
    ...
}

@parkerholladay
Copy link
Author

Strictly speaking, the Result type, is already a HitResult, which happens to contain fields from the raw HTTP response. That being said, I do like your suggestion of an abstraction for the result types, @peterbourgon, where each implementation can contain its success criteria.

The only trouble I see is, that abstraction may make this a bigger change not just for encode, but decode, metrics, and reporting as well. It will possibly mean some breaking changes for lib users on anything that uses the result type. How do you feel about adding this result abstraction @tsenart?

@peterbourgon
Copy link
Contributor

Strictly speaking, the Result type, is already a HitResult, which happens to contain fields from the raw HTTP response.

Vegeta is — or, until this PR, was — an HTTP load testing tool. The Result type didn't happen to contain fields from the raw HTTP response, it explicitly modeled a raw HTTP response.

The only trouble I see is, that abstraction may make this a bigger change not just for encode, but decode, metrics, and reporting as well.

Absolutely true. But this PR is a big change! It changes a fundamental invariant of the tool.

@tsenart
Copy link
Owner

tsenart commented Aug 5, 2023

will change the signature of the encode function across the app to look something like

We shouldn't have to change any signatures. In encode, if a -protocol flag is defined, you'd decode the results from the input as you'd normally do, then, given some top-level function like:

// AsGraphQL re-interprets the given Result with GraphQL semantics, mutating it accordingly.
func AsGraphQL(r *Result) error { ... }

You have everything you need to re-interpret that result as GraphQL in the struct — response headers, body, status code, etc. Then based on the errors in the response body, set the Error field as needed.

Comment on lines +317 to +339
func AsGraphQL(r *Result) error {
if r.Code < 200 || r.Code >= 400 {
return nil
}

var res GQLResponse
err := json.Unmarshal(r.Body, &res)
if err != nil {
return err
}

if res.Errors != nil && len(res.Errors) > 0 {
for i, e := range res.Errors {
if i == 0 {
r.Error = e.Message
} else {
r.Error = fmt.Sprintf("%v, %v", r.Error, e.Message)
}
}
}

return nil
}
Copy link
Author

Choose a reason for hiding this comment

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

@tsenart, let me know if this is more along the lines of what you were hoping to see. I left AsGraphQL() here so I could keep the test checking for mutations of the result object.

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.

4 participants