common: Restore original members of overridden objects after some tests

[kjin]

This change adds a number of methods that query the metadata service. This is necessary for the trace agent, which packages this information in trace data, along with possibly other agents.

The source is mostly from cloud-diagnostics-common, where it made up a portion of utility functions. I felt that util.js here was too large, and would benefit from not having additional methods (or possibly being split up into a number of different files), so I created a separate file and export - but I'd be glad to keep this open for discussion.

In addition, I've modified tests for paginator and util to completely remove monkeypatching after they are completed, since this introduces a side effect where new errors in util.js (ApiError, etc.) would not be instances of the Error class.

PTAL

[stephenplusplus]

Thanks for the PR! I didn't realize the overriding was having a lasting effect. What's the quickest way I can reproduce?

As for the functionality of this PR, I just opened an issue on google-compute-metadata. That library does exactly this, however it is a bit simple in its current form. If the maintainer is okay with targeting individual properties, I think a PR there would be the best way to go. (Unless you don't mind how it works currently, and just want to use it as is)

[kjin]

You can reproduce it by checkout out at f1ea5c9 and running npm test in the common package directory. An assertion in the test for metadata.js that checks whether a returned err object is an instance of Error will fail.

As for using google-compute-metadata - does introducing a PR there mean that we will add a dependency on that library?

+ @ofrobots

[stephenplusplus]

Thank you, I will try that out soon. As for taking on the dependency, yes, it would be used here.

[ofrobots]

Looking at the issues, over at google-compute-metadata, I am concerned about whether it is an actively maintained project, and about the the lack of tests, lack of LICENSE file (the package.json does list MIT license – not sure if that is adequate.)

Other things to think about (I don't have strong feelings here): should the backoff strategy used in this module match the backoff used for metadata requests?

[kjin]

I agree with Ali concerning bringing in a dependency on google-compute-metadata.

I'm not sure if there is anything different about a metadata request over a normal network request, but I don't see any particularly motivating reason to not use the exponential backoff strategy.

[stephenplusplus]

No problem, we can send a PR to address the concerns over there. If we don't get a quick response, we can publish it separately. It's a small task, which is likely why tests weren't added. But the reason I like it in a module as opposed to buried in our internal code is because it's something that is definitely useful to multiple applications. Keeping such functionality outside of this repo is an important philosophy we follow here. It helps us deflect testing / docs / bugs / maintenance to a hyper-focused repository.

Some other priorities will keep me from doing this as quickly as I'd like; would one of you be willing to submit a PR to add a LICENSE file, tests, and the missing features (if you want exponential backoff, use retry-request)*? It should be a pretty quick effort. If not, I'm happy to get to it as soon as I can. In the meantime while developing, maybe use google-compute-metadata in its current form (which pulls all metadata properties), and we can just update the code later.

I really do want to take care of all of this today, I'm just a bit locked up in another task for the near future.

* using retry-request:

var request = require('retry-request')

// same signature as `request` module, but with built in exponential backoff
request('...', function() {})

[ofrobots]

My concern with retry wasn't about the percise strategy gets used by this module, but the fact that this module takes config options (e.g. autoRetry and maxRetries) that wouldn't apply to metadata requestes.

[stephenplusplus]

My concern with retry wasn't about the percise strategy gets used by this module, but the fact that this module takes config options (e.g. autoRetry and maxRetries) that wouldn't apply to metadata requestes.

Okay, I thought the concern was if we use the external module, we won't have any retries performed at all. I think since we're just talking about hitting the metadata server, no precise retry logic is required, regarding autoRetry and maxRetries, so long as we are in fact retrying.

These metadata calls are happening internally, with requests that our code sends, right? Or are the new modules exposing metadata accessors in the public API?

[ofrobots]

The calls to metadata would be private to our code.

I don't have strong feelings on whether autoRetry / maxRetry apply to metadata requests. I'm +1 on modularization in general. I think question for you is whether you care about whether autoRetry/maxRetry preference from the users should apply, and if so, it may suggest that we need our own module for metadata queries (or we should put the API in common).

[stephenplusplus]

I don't think that's as important, since these are calls we're making from our library, so it's up to us to decide what settings are appropriate for our own calls. I think the default settings from retry-request is fine for this. I'm excited to jump on it, but if it starts to impede your workflow, feel free to get a head start.

[ofrobots]

@stephenplusplus given that we haven't seen a response on iamat/google-compute-metadata#9, what do you suggest as the path ahead. @kjin or I would be happy to put together the code/PR.

[stephenplusplus]

I threw this together quickly: https://github.com/stephenplusplus/gcp-metadata -- please check if it works as intended. And if you have a better API in mind, feel free to implement. I added both you and @kjin as owners, so feel free to expand on anything there

[kjin]

Nice, thanks for making this! I'll give it a try.

[stephenplusplus]

Np, thanks! I think I missed the /?recursive option that the other project implemented. Truly, I'm not very well-versed with the metadata server, so if you catch other things like that, please open an issue or just fix it, whichever is easiest.

[kjin]

I'm going to add gcp-metadata as a dependency in the diagnostics agents that need it. Since it supersedes the changes here I'm going to close this PR.

@stephenplusplus could you release the package on npm?

[stephenplusplus]

Definitely, have you checked that it works in the real environment?

[kjin]

Actually, going to re-open this to at least get the changes for reducing side-effects in unit tests checked in.

[kjin]

Yes, I've experimented with it in a GCE instance.

[stephenplusplus]

Cool, thank you. Published [email protected]

[kjin]

@stephenplusplus I know it's now a minor change, but is this fit for merging?

[googlebot]

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

[stephenplusplus]

Can you take a look at my edit? I'm pretty sure if we just clone the require('util.js') part with extend, we should avoid any clobbering.