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.

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

logging: change resource argument to metadata #1666

Merged
merged 4 commits into from
Oct 14, 2016

Conversation

stephenplusplus
Copy link
Contributor

Fixes #1348

Before

var resource = {
  type: 'gce_instance',
  labels: {
    zone: 'global',
    instance_id: '3'
  }
}

var entry = log.entry(resource, data)
entry.otherMetadataProperty = '...' // <- not documented this way, because it wasn't intended to be done this way

After

var metadata = {
  otherMetadataProperty: '...', // <- logical place for this
  resource: {
    type: 'gce_instance',
    labels: {
      zone: 'global',
      instance_id: '3'
    }
  }
}

var entry = log.entry(metadata, data)

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 3, 2016
@stephenplusplus stephenplusplus added the api: logging Issues related to the Cloud Logging API. label Oct 3, 2016
@stephenplusplus
Copy link
Contributor Author

stephenplusplus commented Oct 3, 2016

@zbjornson mind taking a quick look? Thanks for the idea!

@@ -32,7 +32,7 @@ var is = require('is');
* @alias module:logging/entry
* @constructor
*
* @param {object=|string=} resource - See a
* @param {object=} metadata - See a
* [Monitored Resource](https://cloud.google.com/logging/docs/api/ref_v2beta1/rest/v2beta1/MonitoredResource).

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@@ -206,22 +208,24 @@ Log.prototype.emergency = function(entry, options, callback) {
*
* @resource [LogEntry JSON representation]{@link https://cloud.google.com/logging/docs/api/ref_v2beta1/rest/v2beta1/LogEntry}
*
* @param {object=|string=} resource - See a
* @param {object=} metadata - See a
* [Monitored Resource](https://cloud.google.com/logging/docs/api/ref_v2beta1/rest/v2beta1/MonitoredResource).

This comment was marked as spam.

@zbjornson
Copy link
Contributor

Don't have enough time to try it out at the moment, but the API and quick glance at the code look good. Thanks! Let me know if you want me to test before merging, but it looks straightforward enough.

@callmehiphop
Copy link
Contributor

Can we add some system tests for this?

@stephenplusplus
Copy link
Contributor Author

Yes, good idea. Added!

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 2e3f9b7 on stephenplusplus:spp--1348 into fc7ebb8 on GoogleCloudPlatform:master.

@zbjornson
Copy link
Contributor

Thanks for the fix.

Do you know what version this will come out in -- 0.42.0 or 0.41.3? It's a breaking change and I need to update a dependency accordingly.

@stephenplusplus
Copy link
Contributor Author

RE: the comment on mlazarov/bunyan-stackdriver#6, we do follow semver. It'll be 0.42.0, although we're backed up for a while (3-7 days) trying to get promises in. It'll also release independently in @google-cloud/logging@1.0.0-beta.

@zbjornson
Copy link
Contributor

Thanks.

we do follow semver

Er, right -- I guess because it's pre-1.0.0 breaking changes are allowed. :)

@stephenplusplus
Copy link
Contributor Author

Pre-1.0, a minor bump signals a breaking change. npm install --save google-cloud saves the value ^0.41.0 in package.json, which is >=0.41.0 && < 0.42.0. Compared to if we were post 1.0, ^1.41.0 would mean >= 1.41.0 && < 2.0.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: logging Issues related to the Cloud Logging API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants