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.

Sign up for GitHub

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

Jump to bottom

Add support to initialize azure exporters with proxies #902

Merged
merged 1 commit into from
Jun 12, 2020
Merged

Add support to initialize azure exporters with proxies #902

merged 1 commit into from
Jun 12, 2020

Conversation

skinamdar
Copy link
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

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
Contributor Author

@googlebot I fixed it.

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@skinamdar
Copy link
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 c24t, reyang, songy23 and a team as code owners June 11, 2020 01:47
lzchen
lzchen reviewed Jun 11, 2020
View reviewed changes
contrib/opencensus-ext-azure/opencensus/ext/azure/common/transport.py Outdated
@@ -42,6 +42,10 @@ def _transmit(self, envelopes):
This function should never throw exception.
"""
try:
proxies = None
Copy link
Contributor

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
Contributor Author

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
Contributor

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
Contributor Author

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
Contributor

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
Contributor Author

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
Contributor

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
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!

@skinamdar
Copy link
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
Copy link
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
@@ -95,7 +95,7 @@ def __init__(self, *args, **kwargs):
logging_sampling_rate=1.0,
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
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
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
Contributor Author

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
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
Copy link
Contributor Author

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
Contributor Author

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

lzchen commented Jun 12, 2020

I'll probably do a release later next week.

@skinamdar
Copy link
Contributor Author

Sounds good, thank you.

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

@lzchen lzchen lzchen approved these changes

@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 hectorhdzg is a code owner

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