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

Add support to initialize azure exporters with proxies#902

Merged
lzchen merged 1 commit intocensus-instrumentation:masterfrom
skinamdar:add_support_for_proxy_option
Jun 12, 2020
Merged

Add support to initialize azure exporters with proxies#902
lzchen merged 1 commit intocensus-instrumentation:masterfrom
skinamdar:add_support_for_proxy_option

Conversation

@skinamdar
Copy link
Copy Markdown
Contributor

@skinamdar skinamdar commented Jun 11, 2020 •
edited
Loading

Context:
Proxy functionality when instantiating azure exporters is currently unsupported so I had created issue #896. To expedite this feature request, I created this PR which supports posting requests with proxies that are passed in.

Testing:

  • Tested locally to ensure proxy options were set.

  • Tested using nox 3.6 - all tests pass. Note that there are existing linting issues for files unrelated to this PR. To decouple changes, I did not address them in the PR. However, I can fix those linting issues if the maintainers prefer that in this PR.
    flake_8_error

  • Nox status

nox > Session docs was successful.
nox > Ran multiple sessions:
nox > * unit-2.7: failed
nox > * unit-3.5: success
nox > * unit-3.6: success
nox > * system-2.7: skipped
nox > * system-3.6: skipped
nox > * lint: failed
nox > * lint_setup_py: success
nox > * docs: success

Additional Notes:

  • Running nox with python 2.7 fails on my computer when trying to install contrib/opencensus-ext-grpc - I'm still troubleshooting that but wanted to create this PR as it's been tested with python3 at least. If the maintainers could help run this PR on their local machine with nox to ensure python2 compatibility that would be super appreciated as well.

@skinamdar skinamdar requested a review from lzchen as a code owner June 11, 2020 00:55
@googlebot
Copy link
Copy Markdown

googlebot commented Jun 11, 2020

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@skinamdar skinamdar mentioned this pull request Jun 11, 2020
Closed
@skinamdar
Copy link
Copy Markdown
Contributor Author

skinamdar commented Jun 11, 2020

@googlebot I fixed it.

@googlebot
Copy link
Copy Markdown

googlebot commented Jun 11, 2020

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@skinamdar
Copy link
Copy Markdown
Contributor Author

skinamdar commented Jun 11, 2020 •
edited
Loading

I see the linting issues are causing CI to fail - I will fix those items now. I also see CI takes care of running tests against the python2 environment as well.

@skinamdar skinamdar requested review from a team, c24t, reyang and songy23 as code owners June 11, 2020 01:47
lzchen
lzchen reviewed Jun 11, 2020
View reviewed changes
lzchen
lzchen reviewed Jun 11, 2020
View reviewed changes
contrib/opencensus-ext-azure/opencensus/ext/azure/common/transport.py Outdated
This function should never throw exception.
"""
try:
proxies = None
Copy link
Copy Markdown
Contributor

@lzchen lzchen Jun 11, 2020

Choose a reason for hiding this comment

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

Do you need this line?

Copy link
Copy Markdown
Contributor Author

@skinamdar skinamdar Jun 11, 2020

Choose a reason for hiding this comment

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

I set proxies to None because running a json.loads(None), if no proxies are passed in, results in an exception. Does that make sense?

Copy link
Copy Markdown
Contributor

@lzchen lzchen Jun 11, 2020

Choose a reason for hiding this comment

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

If self.options.proxy is None, are you able to change it to an empty dict string?

Copy link
Copy Markdown
Contributor Author

@skinamdar skinamdar Jun 11, 2020

Choose a reason for hiding this comment

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

Great suggestion, I tested that we're able to send a request with proxies=json.loads("{}"). I will update the code to reflect that by setting the default proxies option to be "{}"

Copy link
Copy Markdown
Contributor

@lzchen lzchen Jun 11, 2020

Choose a reason for hiding this comment

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

Awesome. Make sure to initialize it in process_options instead of having a default {} value for proxies.

Copy link
Copy Markdown
Contributor Author

@skinamdar skinamdar Jun 11, 2020

Choose a reason for hiding this comment

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

I found that I need to initialize that value to "{}" in the Options map because even if we set it to a dictionary in process options , that options.proxies values ends up being converted to a string - I found this when running the unit tests.

So it seems we'd have to keep that json.loads behavior in transport.py to make sure that argument is correctly loaded as a dictionary.

Copy link
Copy Markdown
Contributor

@lzchen lzchen Jun 11, 2020

Choose a reason for hiding this comment

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

You don't set it to a dictionary in process options. You set it to a string with an empty dictionary like you've said: "{}".

lzchen
lzchen approved these changes Jun 11, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@lzchen lzchen left a comment

Choose a reason for hiding this comment

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

LGTM.

I've disabled the CircleCI checks since TravisCI is already testing everything we need. All you need to do is fix the build in TravisCI and then should be good to merge!

lzchen
lzchen reviewed Jun 11, 2020
View reviewed changes
@skinamdar
Copy link
Copy Markdown
Contributor Author

skinamdar commented Jun 11, 2020 •
edited
Loading

LGTM.

I've disabled the CircleCI checks since TravisCI is already testing everything we need. All you need to do is fix the build in TravisCI and then should be good to merge!

Great thanks. Do you know how I can re-start the build? I checked the log to identify if it's a CI error which perhaps restarting will fix it - or an actual failure. I skimmed the logs and the main error I saw was AttributeError: 'ThreadingWSGIServer' object has no attribute '_BaseServer__is_shut_down'
I ran nox locally with 3.7 and tests passed so wanted to see if there's a way to restart the build.
nox3 7

lzchen
lzchen reviewed Jun 11, 2020
View reviewed changes
@lzchen
Copy link
Copy Markdown
Contributor

lzchen commented Jun 11, 2020

@skinamdar
To run the build again, simply push another commit to your PR branch.

@skinamdar skinamdar force-pushed the add_support_for_proxy_option branch from e3ef0cd to 6f8d1d8 Compare June 11, 2020 19:39
lzchen
lzchen reviewed Jun 11, 2020
View reviewed changes
contrib/opencensus-ext-azure/opencensus/ext/azure/common/__init__.py Outdated
max_batch_size=100,
minimum_retry_interval=60, # minimum retry interval in seconds
proxy=None,
proxies='{}', # string maps url schemes to the url of the proxies
Copy link
Copy Markdown
Contributor

@lzchen lzchen Jun 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think I was clear in my previous explanation. You keep proxies=None in the constructor, but in process_options you check if proxies is None and you set it to "{}" in that function. This is similar to what we are doing for instrumentatoin_key.

@lzchen
Copy link
Copy Markdown
Contributor

lzchen commented Jun 11, 2020

Also, can you add an entry into CHANGEOG.md under opencensus-ext-azure?

@skinamdar skinamdar changed the title Add support to pass proxy when initializing azure exporters Add support to initialize azure exporters with proxies Jun 11, 2020
@skinamdar skinamdar force-pushed the add_support_for_proxy_option branch 2 times, most recently from cace381 to b1d46e0 Compare June 11, 2020 21:58
@skinamdar
Copy link
Copy Markdown
Contributor Author

skinamdar commented Jun 11, 2020

Currently blocked in travis ci for python2.7 and python3.4 builds due to #905

Still trying to investigate why the python3.7 build is failing since it's not clear from the logs

@lzchen
Copy link
Copy Markdown
Contributor

lzchen commented Jun 12, 2020

@skinamdar
If you rebase with master, builds should pass

@skinamdar skinamdar force-pushed the add_support_for_proxy_option branch from b1d46e0 to 8606482 Compare June 12, 2020 04:40
@skinamdar skinamdar requested a review from hectorhdzg as a code owner June 12, 2020 04:40
@skinamdar
Copy link
Copy Markdown
Contributor Author

skinamdar commented Jun 12, 2020

Awesome, I see the build passed now. Thank you @lzchen for helping fix the builds - can you confirm I'm all set to merge now then?

@lzchen lzchen merged commit 08c34fc into census-instrumentation:master Jun 12, 2020
@skinamdar
Copy link
Copy Markdown
Contributor Author

skinamdar commented Jun 12, 2020

@lzchen Thank you for merging the changes to master. Do you know when the next release of opencensus-ext-azure will be in pip so we can pull in these changes when developing?

@lzchen
Copy link
Copy Markdown
Contributor

lzchen commented Jun 12, 2020

I'll probably do a release later next week.

@skinamdar
Copy link
Copy Markdown
Contributor Author

skinamdar commented Jun 12, 2020

Sounds good, thank you.

@lzchen lzchen mentioned this pull request Jun 17, 2020
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@c24t c24t Awaiting requested review from c24t

@reyang reyang Awaiting requested review from reyang

@songy23 songy23 Awaiting requested review from songy23

@hectorhdzg hectorhdzg Awaiting requested review from hectorhdzg

1 more reviewer

@lzchen lzchen lzchen approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@skinamdar @googlebot @lzchen