Skip to content
This repository was archived by the owner on Sep 17, 2025. It is now read-only.

Hotfix/django flask pyramid status code#755

Merged
reyang merged 30 commits intocensus-instrumentation:masterfrom
victoraugustolls:hotfix/django-flask-pyramid-status-code
Aug 21, 2019
Merged

Hotfix/django flask pyramid status code#755
reyang merged 30 commits intocensus-instrumentation:masterfrom
victoraugustolls:hotfix/django-flask-pyramid-status-code

Conversation

@victoraugustolls
Copy link
Copy Markdown
Contributor

Updating http.status_code to be an int, as discussed with @reyang. This affects the following contributions:

  • Django
  • Flask
  • Httplib
  • Pyramid

Copy link
Copy Markdown
Contributor

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM

@reyang
Copy link
Copy Markdown
Contributor

reyang commented Aug 7, 2019

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.

@victoraugustolls
Copy link
Copy Markdown
Contributor Author

I can update the tests to get the attributes instead, if possible!

@reyang
Copy link
Copy Markdown
Contributor

reyang commented Aug 7, 2019

I haven't done the archaeology, would be curious to understand why we had labels instead of attributes in the first place.

@victoraugustolls
Copy link
Copy Markdown
Contributor Author

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!

@reyang
Copy link
Copy Markdown
Contributor

reyang commented Aug 7, 2019

I don't think it's blocking unless @c24t says otherwise.

@victoraugustolls
Copy link
Copy Markdown
Contributor Author

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!

@reyang reyang mentioned this pull request Aug 7, 2019
@victoraugustolls
Copy link
Copy Markdown
Contributor Author

@c24t any comments here?

@victoraugustolls
Copy link
Copy Markdown
Contributor Author

Pinging @c24t, if there are no responses, should we merge @reyang ?

@reyang
Copy link
Copy Markdown
Contributor

reyang commented Aug 13, 2019

@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.

@victoraugustolls
Copy link
Copy Markdown
Contributor Author

Will update later today!

Copy link
Copy Markdown
Contributor

@songy23 songy23 left a 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.

@victoraugustolls
Copy link
Copy Markdown
Contributor Author

@songy23 and what about the failing system tests?

@victoraugustolls
Copy link
Copy Markdown
Contributor Author

A course of action is needed here, system tests fails but the changes were approved. @c24t @songy23 @reyang

@reyang
Copy link
Copy Markdown
Contributor

reyang commented Aug 16, 2019

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?

@victoraugustolls
Copy link
Copy Markdown
Contributor Author

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!

@victoraugustolls
Copy link
Copy Markdown
Contributor Author

@c24t @songy23 @reyang added tests to see if status code was being converted correctly, but still not enough for the system tests. Can someone take a look?

reyang added a commit that referenced this pull request Aug 20, 2019
@victoraugustolls
Copy link
Copy Markdown
Contributor Author

Merged @reyang solution, thanks for the help!
@songy23 @c24t we are receiving 429 from Google Client here, any tips?

@reyang reyang merged commit 52b974f into census-instrumentation:master Aug 21, 2019
@victoraugustolls victoraugustolls deleted the hotfix/django-flask-pyramid-status-code branch August 22, 2019 17:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants