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

fixed more 0.14b0 breakages and updated tests to catch them #83

Merged
merged 1 commit into from
Oct 26, 2020
Merged

fixed more 0.14b0 breakages and updated tests to catch them #83

merged 1 commit into from
Oct 26, 2020

Conversation

aabmass
Copy link
Collaborator

@aabmass aabmass commented Oct 23, 2020 •
edited
Loading

There were some more breakages from v0.14b0 that needed fixing before release. Most of this PR is updating tests to actually catch the breakages in the future.

  • Updated unit tests to use autospeccing
  • Update the integration tests against GCM Mock api to catch the errors too
    • turns out the cloud monitoring integration tests were passing even though they shouldn't, because we swallow exceptions when CreateMetricDescriptor() fails
    • updated tests to fail if there is an error logged from CreateMetricDescriptor() failure
    • updated tests/exporter to actually pass
  • Updated changelogs

@google-cla google-cla bot added the cla: yes label Oct 23, 2020
@aabmass aabmass force-pushed the fix-resource-access branch 2 times, most recently from e26bbef to 1402ce5 Compare October 23, 2020 23:20
@aabmass aabmass mentioned this pull request Oct 23, 2020
Open
@aabmass aabmass marked this pull request as ready for review October 23, 2020 23:21
@aabmass aabmass requested a review from a team October 23, 2020 23:21
@aabmass aabmass force-pushed the fix-resource-access branch from 1402ce5 to 3556d92 Compare October 23, 2020 23:26
@aabmass aabmass force-pushed the fix-resource-access branch from 3556d92 to ca58887 Compare October 23, 2020 23:27
nilebox
nilebox approved these changes Oct 25, 2020
View reviewed changes
opentelemetry-exporter-google-cloud/tests/test_cloud_monitoring_exporter_integration.py
"cloud.provider": "gcp",
"cloud.zone": "us-east1-b",
"host.id": 654321,
"gcp.resource_type": "gce_instance",
Copy link
Contributor

Choose a reason for hiding this comment

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

Have we decided to keep this one? It's incompatible with semantic conventions, but might be okay as an additional attribute.

Also, IIRC you shared a link where AWS has introduced attributes / resources specific to AWS to the spec? Shall we try doing the same if we want to support GCP-specific resource attributes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Have we decided to keep this one? It's incompatible with semantic conventions, but might be okay as an additional attribute.

I think it's better to use the semantic conventions if we aren't missing any functionality #85. Do you have any preference on adding our own GCP resource attributes instead? (Here's the AWS PR for reference open-telemetry/opentelemetry-specification#1099))

@aabmass aabmass merged commit eea24af into GoogleCloudPlatform:master Oct 26, 2020
@aabmass aabmass deleted the fix-resource-access branch October 26, 2020 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nilebox nilebox nilebox approved these changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@aabmass @nilebox