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

examples: add gcp-observability examples #9967

Merged
merged 8 commits into from
Mar 23, 2023

Conversation

DNVindhya
Copy link
Contributor

@DNVindhya DNVindhya commented Mar 20, 2023

This PR adds gcp-observability example based on HelloWorld.

CC @sanjaypujare @ejona86 @fengli79


The GCP Observability example consists of a Hello World client and a Hello World server instrumented for logs, metrics and tracing.

__Please refer to GCP observability user guide for setting up authorization to acess Google Cloud Platform with Google user credentials.__
Copy link
Contributor

@sanjaypujare sanjaypujare Mar 20, 2023

Choose a reason for hiding this comment

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

Can we provide a link to the UG? We have one for preview and it's not going to change for GA, right?

Also it will be good if the link is to the specific section about credentials and authorization

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 checked with Lisa, she is trying to get a new directory set up. In that case, UG link will be different than what we have now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, we can go with the current link and update it whenever we have the updated link

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated and added current user guide link.

Copy link
Contributor

Choose a reason for hiding this comment

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

Link is correct but the link text "GCP observability user guide" doesn't match it or is not even correct. The user guide says it is for "Microservices observability" - so we could at least say that. In reality the UG is for "gRPC GCP Observability" or "gRPC Microservices on GCP Observability" . "GCP Observability" is too broad.

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 removed the link, as the link is going to be changed when we release.
The product name is "Microservices Observability", should i add "gRPC Microservices Observability" instead here to show connection to gRPC (while we do not have a link). Let me know what you think.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you removed the link, make sure the description is as close to what the actual UG title is (since users will have to search to find it). I think it is "Microservices Observability" so that makes most sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

String.format(
"Sleeping %d seconds before shutdown to ensure all records are flushed.",
EXPORT_INTERVAL));
Thread.sleep(TimeUnit.MILLISECONDS.convert(EXPORT_INTERVAL, TimeUnit.SECONDS));
Copy link
Member

Choose a reason for hiding this comment

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

If a sleep is still necessary, we should have that sleep in gcp-observability. The user shouldn't have to deal with stuff like this. Once we fix the issue the user's shutdown would naturally run faster.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, will add sleep to gcp-observability in a follow up PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in PR #9972

Copy link
Contributor

@sanjaypujare sanjaypujare left a comment

Choose a reason for hiding this comment

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

Added a bunch of comments. Will take a look again once addressed

@DNVindhya
Copy link
Contributor Author

Addressed comments. @sanjaypujare PTAL.

@ejona86 ejona86 added the TODO:release blocker Issue/PR is important enough to delay the release. Removed after release issues resolved label Mar 22, 2023
*/
public static void main(String[] args) throws IOException, InterruptedException {
// Initialize observability
GcpObservability observability = GcpObservability.grpcInit();
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we do?

try(observability) {

} finally {
WaitUntilShutdown();
}

Copy link
Contributor Author

@DNVindhya DNVindhya Mar 22, 2023

Choose a reason for hiding this comment

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

Do you mean

try (observability) {
   final GcpObservabilityServer server = new GcpObservabilityServer();
} finally {
  server.blockUntilShutdown();
}

In this case, server being a non-static variable cannot be accessed in a static method. Server needs to be created after initializing observability within the try block.

Copy link
Contributor

Choose a reason for hiding this comment

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

In the above code server is not in scope in the finally clause ...

{
"cloud_monitoring": {},
"cloud_trace": {
"sampling_rate": 1.00
Copy link
Contributor

Choose a reason for hiding this comment

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

1.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

@sanjaypujare sanjaypujare left a comment

Choose a reason for hiding this comment

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

I have some comments and it will be nice to address them but looks good so okay to merge without them too.

@sanjaypujare sanjaypujare merged commit af8048b into grpc:master Mar 23, 2023
DNVindhya added a commit to DNVindhya/grpc-java that referenced this pull request Mar 23, 2023
* add examples for gcp-observability
sanjaypujare pushed a commit that referenced this pull request Mar 23, 2023
@ejona86 ejona86 removed the TODO:release blocker Issue/PR is important enough to delay the release. Removed after release issues resolved label Mar 23, 2023
larry-safran pushed a commit to larry-safran/grpc-java that referenced this pull request Mar 23, 2023
* add examples for gcp-observability
@DNVindhya DNVindhya deleted the gcp-o11y-examples branch March 24, 2023 20:45
larry-safran pushed a commit to larry-safran/grpc-java that referenced this pull request Mar 29, 2023
* add examples for gcp-observability
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants