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

Include operation_id in error strings #150

Closed
cben opened this issue Feb 16, 2020 · 2 comments
Closed

Include operation_id in error strings #150

cben opened this issue Feb 16, 2020 · 2 comments

Comments

@cben
Copy link
Contributor

cben commented Feb 16, 2020

Here's an example of a pretty idiomatic SDK usage:
https://github.com/openshift/osde2e/blob/143f353ea48119e9dc3e65868cc2466e738c9943/pkg/osd/cluster.go#L74-L103
(where u.clusters() returns u.conn.ClustersMgmt().V1().Clusters())

This code eventually logged the following error:

failed waiting for cluster ready: Encountered error waiting for cluster: couldn't get cluster '1b9b8un2n10cbv8l5oaq2ek0m5fdf07p': couldn't retrieve cluster '1b9b8un2n10cbv8l5oaq2ek0m5fdf07p': CLUSTERS-MGMT-500

where most of the error is from the calling osde2e code, apparently CLUSTERS-MGMT-500 is the only part that came from the SDK.

In this case, I failed to find matching clusters-service logs to this 500 (interal hive-integration env).
Almost always 500 errors include an "operation_id": "...." field, and these make later searching much easier.
I think it would be good if the SDK included such id, when available, in the error string it returns.

I guess the reason we didn't do this already is our internal code using the SDK tends to log the full response body on errors. In osde2e code, the immediate function returns the body too:

return resp.Body(), resp.Error()

but then the calling function ignores the body because err != nil. Which I think is likely to happen in real-world usage, frequently only the error string propogates out...

@jhernand what do you think? cc @meowfaceman @ethernetdan from osde2e

@mdwn
Copy link

mdwn commented Feb 24, 2020

Just reviving this a bit -- we've seen this a few times recently. I think this is a good idea. cc @jeefy (@ethernetdan is no longer on osde2e).

jhernand added a commit to jhernand/ocm-api-metamodel that referenced this issue Feb 26, 2020
This patch changes the code generator so that errors support the
`operation_id` attribute. This attribute and the existing ones will now
be used to generate more complete error messages. For example, the
following JSON error response:

```
{
  "kind": "Error",
  "id": "401",
  "href": "/api/clusters_mgmt/v1/errors/401",
  "code": "CLUSTERS-MGMT-401",
  "reason": "My reason",
  "operation_id": "456"
}
```

Will result in the following error string (in one single line):

```
identifier is '401', code is 'CLUSTERS-MGMT-401' and
operation identifier is '456': My reason
```

Related: openshift-online/ocm-sdk-go#150
Signed-off-by: Juan Hernandez <[email protected]>
jhernand added a commit to jhernand/ocm-api-metamodel that referenced this issue Feb 26, 2020
This patch changes the code generator so that errors support the
`operation_id` attribute. This attribute and the existing ones will now
be used to generate more complete error messages. For example, the
following JSON error response:

```
{
  "kind": "Error",
  "id": "401",
  "href": "/api/clusters_mgmt/v1/errors/401",
  "code": "CLUSTERS-MGMT-401",
  "reason": "My reason",
  "operation_id": "456"
}
```

Will result in the following error string (in one single line):

```
identifier is '401', code is 'CLUSTERS-MGMT-401' and
operation identifier is '456': My reason
```

Related: openshift-online/ocm-sdk-go#150
Signed-off-by: Juan Hernandez <[email protected]>
jhernand added a commit to jhernand/ocm-api-metamodel that referenced this issue Feb 26, 2020
This patch changes the code generator so that errors support the
`operation_id` attribute. This attribute and the existing ones will now
be used to generate more complete error messages. For example, the
following JSON error response:

```
{
  "kind": "Error",
  "id": "401",
  "href": "/api/clusters_mgmt/v1/errors/401",
  "code": "CLUSTERS-MGMT-401",
  "reason": "My reason",
  "operation_id": "456"
}
```

Will result in the following error string (in one single line):

```
identifier is '401', code is 'CLUSTERS-MGMT-401' and
operation identifier is '456': My reason
```

Related: openshift-online/ocm-sdk-go#150
Signed-off-by: Juan Hernandez <[email protected]>
jhernand added a commit to jhernand/ocm-api-metamodel that referenced this issue Feb 26, 2020
The more relevant changes in the new version are the following:

- Add `operation_id` attribute to error objects and error messages.

Related: openshift-online/ocm-sdk-go#150
Signed-off-by: Juan Hernandez <[email protected]>
jhernand added a commit to jhernand/ocm-sdk-go that referenced this issue Feb 26, 2020
The more relevant changes in the new version of the metamodel are the
following:

- Add `operation_id` attribute to error objects and error messages.

Related: openshift-online#150
Signed-off-by: Juan Hernandez <[email protected]>
jhernand added a commit to jhernand/ocm-sdk-go that referenced this issue Feb 26, 2020
The more relevant changes in the new version are the following:

- Update to metamodel 0.0.26:
** Add `operation_id` attribute to error objects and error messages.

Related: openshift-online#150
Signed-off-by: Juan Hernandez <[email protected]>
@jhernand
Copy link
Collaborator

This is fixed in release 0.1.89. For a JSON error document like this:

{
  "kind": "Error",
  "id": "401",
  "href": "/api/clusters_mgmt/v1/errors/401",
  "code": "CLUSTERS-MGMT-401",
  "reason": "My reason",
  "operation_id": "456"
}

The Error and String methos of the error type of the SDK will no return someting like this:

identifier is '401', code is 'CLUSTERS-MGMT-401' and
operation identifier is '456': My reason

tzvatot pushed a commit to tzvatot/ocm-sdk-go that referenced this issue Apr 5, 2020
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

No branches or pull requests

3 participants