-
Notifications
You must be signed in to change notification settings - Fork 250
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
Hotfix/django flask pyramid status code #755
Hotfix/django flask pyramid status code #755
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I haven't taken a deep look into the system test failures. It seems some of the system tests are looking for span labels instead of attributes, and label value is always string. |
I can update the tests to get the attributes instead, if possible! |
I haven't done the archaeology, would be curious to understand why we had labels instead of attributes in the first place. |
Who might know this? And is this a block for merging? If so, I can either swap for attributes or try to understand why we are using labels! |
I don't think it's blocking unless @c24t says otherwise. |
Waiting for his input so, and if is a block, I would please ask if @c24t can give an explanation of why we are using labels and if there is any workaround! |
@c24t any comments here? |
@victoraugustolls sorry for my delayed response. We can move forward by changing the test cases to use attributes instead of labels. Once the CI passed, we're good to merge. |
Will update later today! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Stackdriver should use int for status code too.
@songy23 and what about the failing system tests? |
It seems that Stackdriver labels are always strings. @victoraugustolls would the system test pass if you convert status from int to str in the Stackdriver exporter? |
Google client only accepts labels as strings due to the proto format, and during the conversion inside the client, it loses the status code. If I transform the status code to string, it would work, but wouldn’t be “right”. But I think this could be tracked in another issue so we can move this forward! |
Updating
http.status_code
to be an int, as discussed with @reyang. This affects the following contributions: