Add errors to spans

[ankur22]

What?

Adding errors to the API spans that currently exist.

Why?

When a user views the traces for the test, it's useful to see whether an error occurred and in which API call. This will help highlight spans where errors occurred and therefore caused the test iteration to fail.

Checklist

• I have performed a self-review of my code
• I have added tests for my changes
• I have commented on my code, particularly in hard-to-understand areas

Related PR(s)/Issue(s)

#1249

[andrewslotin]

Left a few minor comments. @inancgumus, WDYT?

[inancgumus]

I'm happy with the overall PR 👍

I have some wonderings:

1. Why is SpanRecordError exported?
2. Can we use the existing error? Is there a reason for repurposing a new error message? Should we repurpose the original error and directly use it instead?
3. Weak suggestion: Can we register span errors in the mapping layer (instead of intermingling or handling them into the core logic)?

[ankur22]

I'm happy with the overall PR 👍

I have some wonderings:

1. Why is SpanRecordError exported?
2. Can we use the existing error? Is there a reason for repurposing a new error message? Should we repurpose the original error and directly use it instead?
3. Weak suggestion: Can we register span errors in the mapping layer (instead of intermingling or handling them into the core logic)?

1. Good spot, i copied the format from the other APIs in the file. Fixed in c0c2a6e
2. I'm not sure what you mean here, Could you give an example?
3. This is something I was thinking about too. I think it's worth experimenting but in another PR. It would involve changing the method signature to always expect a context.Context, which is generally good, but also confusing in our case since the context would only carry the trace information and not be used to terminate the iteration/method 🤔

[inancgumus]

@ankur22

1. Good spot, i copied the format from the other APIs in the file. Fixed in c0c2a6e

👍

2. I'm not sure what you mean here, Could you give an example?

Sure. I mean this (using the existing error message):

err := errors.New("existing browser context must be closed before creating a new one")
spanRecordError(span, err)
return nil, err

Instead of:

err := errors.New("existing browser context must be closed before creating a new one")
spanRecordError(span, "browserContext already exists", err)
return nil, err

In the current version, there are two error messages:

1. "existing browser context must be closed before creating a new one"
2. "browserContext already exists"

I thought maybe we could use a single error message. If the existing error messages are too verbose, we could repurpose them to be compatible with span error messages. Keeping the error messages consistent will prevent confusion in the future and also lead to less code.

3. This is something I was thinking about too. I think it's worth experimenting but in another PR. It would involve changing the method signature to always expect a context.Context, which is generally good, but also confusing in our case since the context would only carry the trace information and not be used to terminate the iteration/method 🤔

Context is also used to carry information. So, it's fine to experiment with it.

[ankur22]

2. I'm not sure what you mean here, Could you give an example?

Sure. I mean this (using the existing error message):

err := errors.New("existing browser context must be closed before creating a new one")
spanRecordError(span, err)
return nil, err

Instead of:

err := errors.New("existing browser context must be closed before creating a new one")
spanRecordError(span, "browserContext already exists", err)
return nil, err

In the current version, there are two error messages:

1. "existing browser context must be closed before creating a new one"
2. "browserContext already exists"

I thought maybe we could use a single error message. If the existing error messages are too verbose, we could repurpose them to be compatible with span error messages. Keeping the error messages consistent will prevent confusion in the future and also lead to less code.

I see now. Adding a description of the error seems to be a requirement when recording the error in the span:

func spanRecordError(span trace.Span, description string, err error) {
	span.SetStatus(codes.Error, description)
	span.RecordError(err)
}

Any other options?

[inancgumus]

@ankur22

I see now. Adding a description of the error seems to be a requirement when recording the error in the span.

Yes, but the description doesn't have to be a different error. For example.

[ankur22]

Yes, but the description doesn't have to be a different error. For example.

Nice find, if that's what they're doing then I will do the same 👍 Changed in 2207e4b