From 737ab94cbc45dff6ea1cbfb64cd8076c8cc3bd1f Mon Sep 17 00:00:00 2001 From: Dennis Granath Date: Fri, 15 Oct 2021 14:27:28 +0200 Subject: [PATCH 1/7] Add Noop tracer and trace calls --- src/main/java/com/spotify/github/Span.java | 29 +++++ src/main/java/com/spotify/github/Tracer.java | 28 +++++ .../github/v3/clients/ChecksClient.java | 32 ++++- .../github/v3/clients/GitDataClient.java | 51 ++++++-- .../github/v3/clients/GithubAppClient.java | 23 +++- .../github/v3/clients/IssueClient.java | 23 +++- .../spotify/github/v3/clients/NoopTracer.java | 52 +++++++++ .../github/v3/clients/PullRequestClient.java | 51 ++++++-- .../github/v3/clients/RepositoryClient.java | 109 +++++++++++++----- .../github/v3/clients/SearchClient.java | 18 ++- 10 files changed, 351 insertions(+), 65 deletions(-) create mode 100644 src/main/java/com/spotify/github/Span.java create mode 100644 src/main/java/com/spotify/github/Tracer.java create mode 100644 src/main/java/com/spotify/github/v3/clients/NoopTracer.java diff --git a/src/main/java/com/spotify/github/Span.java b/src/main/java/com/spotify/github/Span.java new file mode 100644 index 00000000..0cd62565 --- /dev/null +++ b/src/main/java/com/spotify/github/Span.java @@ -0,0 +1,29 @@ +/* + * Copyright (c) 2019 Spotify AB + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not + * use this file except in compliance with the License. You may obtain a copy of + * the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + */ + +package com.spotify.github; + +public interface Span extends AutoCloseable { + + Span success(); + + Span failure(); + + /** Close span. Must be called for any opened span. */ + @Override + void close(); +} + diff --git a/src/main/java/com/spotify/github/Tracer.java b/src/main/java/com/spotify/github/Tracer.java new file mode 100644 index 00000000..cbff07c3 --- /dev/null +++ b/src/main/java/com/spotify/github/Tracer.java @@ -0,0 +1,28 @@ +/* + * Copyright (c) 2019 Spotify AB + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not + * use this file except in compliance with the License. You may obtain a copy of + * the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + */ + +package com.spotify.github; + +import java.util.concurrent.CompletionStage; + +public interface Tracer { + + /** Create scoped span. Span will be closed when future completes. */ + Span span( + final String name, final CompletionStage future); + +} + diff --git a/src/main/java/com/spotify/github/v3/clients/ChecksClient.java b/src/main/java/com/spotify/github/v3/clients/ChecksClient.java index 70439199..ea3d139f 100644 --- a/src/main/java/com/spotify/github/v3/clients/ChecksClient.java +++ b/src/main/java/com/spotify/github/v3/clients/ChecksClient.java @@ -21,6 +21,7 @@ package com.spotify.github.v3.clients; import com.google.common.collect.ImmutableMap; +import com.spotify.github.Tracer; import com.spotify.github.v3.checks.CheckRunRequest; import com.spotify.github.v3.checks.CheckRunResponse; import com.spotify.github.v3.checks.CheckRunResponseList; @@ -47,6 +48,8 @@ public class ChecksClient { private final Map extraHeaders = ImmutableMap.of(HttpHeaders.ACCEPT, "application/vnd.github.antiope-preview+json"); + private Tracer tracer = NoopTracer.INSTANCE; + /** * Instantiates a new Checks client. * @@ -60,6 +63,11 @@ public class ChecksClient { this.repo = repo; } + public ChecksClient withTracer(Tracer tracer) { + this.tracer = tracer; + return this; + } + /** * Create a checkRun. * @@ -68,8 +76,10 @@ public class ChecksClient { */ public CompletableFuture createCheckRun(final CheckRunRequest checkRun) { final String path = String.format(CHECK_RUNS_URI, owner, repo); - return github.post( + CompletableFuture future = github.post( path, github.json().toJsonUnchecked(checkRun), CheckRunResponse.class, extraHeaders); + tracer.span("Create checkrun", future); + return future; } /** @@ -82,8 +92,10 @@ public CompletableFuture createCheckRun(final CheckRunRequest public CompletableFuture updateCheckRun( final int id, final CheckRunRequest checkRun) { final String path = String.format(GET_CHECK_RUN_URI, owner, repo, id); - return github.patch( + CompletableFuture future = github.patch( path, github.json().toJsonUnchecked(checkRun), CheckRunResponse.class, extraHeaders); + tracer.span("Update checkrun", future); + return future; } /** @@ -94,7 +106,9 @@ public CompletableFuture updateCheckRun( */ public CompletableFuture getCheckRun(final int id) { final String path = String.format(GET_CHECK_RUN_URI, owner, repo, id); - return github.request(path, CheckRunResponse.class, extraHeaders); + CompletableFuture future = github.request(path, CheckRunResponse.class, extraHeaders); + tracer.span("Get checkrun", future); + return future; } /** @@ -105,7 +119,9 @@ public CompletableFuture getCheckRun(final int id) { */ public CompletableFuture getCheckRuns(final String ref) { final String path = String.format(LIST_CHECK_RUNS_URI, owner, repo, ref); - return github.request(path, CheckRunResponseList.class, extraHeaders); + CompletableFuture future = github.request(path, CheckRunResponseList.class, extraHeaders); + tracer.span("List checkruns", future); + return future; } /** @@ -116,7 +132,9 @@ public CompletableFuture getCheckRuns(final String ref) { */ public CompletableFuture getCheckSuite(final String id) { final String path = String.format(GET_CHECK_SUITE_URI, owner, repo, id); - return github.request(path, CheckSuite.class, extraHeaders); + CompletableFuture future = github.request(path, CheckSuite.class, extraHeaders); + tracer.span("Get check suite", future); + return future; } /** @@ -127,6 +145,8 @@ public CompletableFuture getCheckSuite(final String id) { */ public CompletableFuture getCheckSuites(final String sha) { final String path = String.format(LIST_CHECK_SUITES_REF_URI, owner, repo, sha); - return github.request(path, CheckSuiteResponseList.class, extraHeaders); + CompletableFuture future = github.request(path, CheckSuiteResponseList.class, extraHeaders); + tracer.span("List check suites", future); + return future; } } diff --git a/src/main/java/com/spotify/github/v3/clients/GitDataClient.java b/src/main/java/com/spotify/github/v3/clients/GitDataClient.java index 760a474c..6729be78 100644 --- a/src/main/java/com/spotify/github/v3/clients/GitDataClient.java +++ b/src/main/java/com/spotify/github/v3/clients/GitDataClient.java @@ -26,6 +26,7 @@ import static java.lang.String.format; import com.google.common.collect.ImmutableMap; +import com.spotify.github.Tracer; import com.spotify.github.v3.git.Reference; import com.spotify.github.v3.git.ShaLink; import com.spotify.github.v3.git.Tag; @@ -57,6 +58,7 @@ public class GitDataClient { private final GitHubClient github; private final String owner; private final String repo; + private Tracer tracer = NoopTracer.INSTANCE; GitDataClient(final GitHubClient github, final String owner, final String repo) { this.github = github; @@ -68,6 +70,11 @@ static GitDataClient create(final GitHubClient github, final String owner, final return new GitDataClient(github, owner, repo); } + public GitDataClient withTracer(Tracer tracer) { + this.tracer = tracer; + return this; + } + /** * Deletes a git reference. * @@ -75,7 +82,9 @@ static GitDataClient create(final GitHubClient github, final String owner, final */ public CompletableFuture deleteReference(final String ref) { final String path = format(REFERENCE_URI, owner, repo, ref.replaceAll("refs/", "")); - return github.delete(path).thenAccept(IGNORE_RESPONSE_CONSUMER); + CompletableFuture future = github.delete(path).thenAccept(IGNORE_RESPONSE_CONSUMER); + tracer.span("Delete reference", future); + return future; } /** @@ -103,7 +112,9 @@ public CompletableFuture deleteTag(final String tag) { */ public CompletableFuture getBranchReference(final String branch) { final String path = format(BRANCH_REFERENCE_URI, owner, repo, branch); - return github.request(path, Reference.class); + CompletableFuture future = github.request(path, Reference.class); + tracer.span("Get branch reference", future); + return future; } /** @@ -113,7 +124,9 @@ public CompletableFuture getBranchReference(final String branch) { */ public CompletableFuture getTagReference(final String tag) { final String path = format(TAG_REFERENCE_URI, owner, repo, tag); - return github.request(path, Reference.class); + CompletableFuture future = github.request(path, Reference.class); + tracer.span("Get tag reference", future); + return future; } /** @@ -123,7 +136,9 @@ public CompletableFuture getTagReference(final String tag) { */ public CompletableFuture getTag(final String tag) { final String path = format(TAG_URI, owner, repo, tag); - return github.request(path, Tag.class); + CompletableFuture future = github.request(path, Tag.class); + tracer.span("Get annotated tag", future); + return future; } /** @@ -133,7 +148,9 @@ public CompletableFuture getTag(final String tag) { */ public CompletableFuture> listMatchingReferences(final String ref) { final String path = format(LIST_MATCHING_REFERENCES_URI, owner, repo, ref); - return github.request(path, LIST_REFERENCES); + CompletableFuture> future = github.request(path, LIST_REFERENCES); + tracer.span("List matching references", future); + return future; } /** @@ -165,7 +182,9 @@ public CompletableFuture createReference(final String ref, final Stri of( "ref", ref, "sha", sha); - return github.post(path, github.json().toJsonUnchecked(body), Reference.class); + CompletableFuture future = github.post(path, github.json().toJsonUnchecked(body), Reference.class); + tracer.span("Create git reference", future); + return future; } /** @@ -235,7 +254,9 @@ public CompletableFuture createCommit( .json() .toJsonUnchecked( ImmutableMap.of("message", message, "parents", parents, "tree", treeSha)); - return github.post(path, requestBody, Commit.class); + CompletableFuture future = github.post(path, requestBody, Commit.class); + tracer.span("Create commit", future); + return future; } /** @@ -246,7 +267,9 @@ public CompletableFuture createCommit( */ public CompletableFuture getTree(final String sha) { final String path = String.format(TREE_SHA_URI_TEMPLATE, owner, repo, sha); - return github.request(path, Tree.class); + CompletableFuture future = github.request(path, Tree.class); + tracer.span("Get repository tree", future); + return future; } /** @@ -257,7 +280,9 @@ public CompletableFuture getTree(final String sha) { */ public CompletableFuture getRecursiveTree(final String sha) { final String path = String.format(TREE_SHA_URI_TEMPLATE, owner, repo, sha); - return github.request(path + "?recursive=true", Tree.class); + CompletableFuture future = github.request(path + "?recursive=true", Tree.class); + tracer.span("Get recursive tree", future); + return future; } /** @@ -271,7 +296,9 @@ public CompletableFuture createTree(final List tree, final Strin final String path = String.format(TREE_URI_TEMPLATE, owner, repo); final String requestBody = github.json() .toJsonUnchecked(ImmutableMap.of("base_tree", baseTreeSha, "tree", tree)); - return github.post(path, requestBody, Tree.class); + CompletableFuture future = github.post(path, requestBody, Tree.class); + tracer.span("Create tree", future); + return future; } @@ -285,7 +312,9 @@ public CompletableFuture createBlob(final String content) { final String encoding = "utf-8|base64"; final String requestBody = github.json() .toJsonUnchecked(ImmutableMap.of("content", content, "encoding", encoding)); - return github.post(path, requestBody, ShaLink.class); + CompletableFuture future = github.post(path, requestBody, ShaLink.class); + tracer.span("Create blob", future); + return future; } diff --git a/src/main/java/com/spotify/github/v3/clients/GithubAppClient.java b/src/main/java/com/spotify/github/v3/clients/GithubAppClient.java index bcc075a5..1992ee5c 100644 --- a/src/main/java/com/spotify/github/v3/clients/GithubAppClient.java +++ b/src/main/java/com/spotify/github/v3/clients/GithubAppClient.java @@ -22,6 +22,7 @@ import com.fasterxml.jackson.core.type.TypeReference; import com.google.common.collect.ImmutableMap; +import com.spotify.github.Tracer; import com.spotify.github.v3.apps.InstallationRepositoriesResponse; import com.spotify.github.v3.checks.AccessToken; import com.spotify.github.v3.checks.Installation; @@ -41,6 +42,7 @@ public class GithubAppClient { private final GitHubClient github; private final String owner; private final String repo; + private Tracer tracer = NoopTracer.INSTANCE; private final Map extraHeaders = ImmutableMap.of(HttpHeaders.ACCEPT, "application/vnd.github.machine-man-preview+json"); @@ -54,13 +56,20 @@ public class GithubAppClient { this.repo = repo; } + public GithubAppClient withTracer(Tracer tracer) { + this.tracer = tracer; + return this; + } + /** * List Installations of an app. * * @return a list of Installation */ public CompletableFuture> getInstallations() { - return github.request(GET_INSTALLATIONS_URL, INSTALLATION_LIST_TYPE_REFERENCE, extraHeaders); + CompletableFuture> future = github.request(GET_INSTALLATIONS_URL, INSTALLATION_LIST_TYPE_REFERENCE, extraHeaders); + tracer.span("List Installations", future); + return future; } /** @@ -69,8 +78,10 @@ public CompletableFuture> getInstallations() { * @return a list of Installation */ public CompletableFuture getInstallation() { - return github.request( + CompletableFuture future = github.request( String.format(GET_INSTALLATION_REPO_URL, owner, repo), Installation.class, extraHeaders); + tracer.span("Get installation", future); + return future; } /** @@ -80,7 +91,9 @@ public CompletableFuture getInstallation() { */ public CompletableFuture getAccessToken(final Integer installationId) { final String path = String.format(GET_ACCESS_TOKEN_URL, installationId); - return github.post(path, "", AccessToken.class, extraHeaders); + CompletableFuture future = github.post(path, "", AccessToken.class, extraHeaders); + tracer.span("Get access token", future); + return future; } /** @@ -92,7 +105,9 @@ public CompletableFuture getAccessToken(final Integer installationI public CompletableFuture listAccessibleRepositories( final int installationId) { - return GitHubClient.scopeForInstallationId(github, installationId) + CompletableFuture future = GitHubClient.scopeForInstallationId(github, installationId) .request(LIST_ACCESSIBLE_REPOS_URL, InstallationRepositoriesResponse.class, extraHeaders); + tracer.span("List scoped repositories", future); + return future; } } diff --git a/src/main/java/com/spotify/github/v3/clients/IssueClient.java b/src/main/java/com/spotify/github/v3/clients/IssueClient.java index 1d965d27..8ef6f684 100644 --- a/src/main/java/com/spotify/github/v3/clients/IssueClient.java +++ b/src/main/java/com/spotify/github/v3/clients/IssueClient.java @@ -24,6 +24,7 @@ import static com.spotify.github.v3.clients.GitHubClient.LIST_COMMENT_TYPE_REFERENCE; import com.google.common.collect.ImmutableMap; +import com.spotify.github.Tracer; import com.spotify.github.async.AsyncPage; import com.spotify.github.v3.comment.Comment; import java.lang.invoke.MethodHandles; @@ -43,6 +44,7 @@ public class IssueClient { private final GitHubClient github; private final String owner; private final String repo; + private Tracer tracer = NoopTracer.INSTANCE; IssueClient(final GitHubClient github, final String owner, final String repo) { this.github = github; @@ -54,6 +56,11 @@ static IssueClient create(final GitHubClient github, final String owner, final S return new IssueClient(github, owner, repo); } + public IssueClient withTracer(Tracer tracer) { + this.tracer = tracer; + return this; + } + /** * List repository comments. * @@ -82,7 +89,9 @@ public Iterator> listComments(final int number) { public CompletableFuture getComment(final int id) { final String path = String.format(COMMENTS_URI_ID_TEMPLATE, owner, repo, id); log.info("Fetching issue comments from " + path); - return github.request(path, Comment.class); + CompletableFuture future = github.request(path, Comment.class); + tracer.span("Get comment", future); + return future; } /** @@ -95,7 +104,9 @@ public CompletableFuture getComment(final int id) { public CompletableFuture createComment(final int number, final String body) { final String path = String.format(COMMENTS_URI_NUMBER_TEMPLATE, owner, repo, number); final String requestBody = github.json().toJsonUnchecked(ImmutableMap.of("body", body)); - return github.post(path, requestBody, Comment.class); + CompletableFuture future = github.post(path, requestBody, Comment.class); + tracer.span("Create comment", future); + return future; } /** @@ -106,9 +117,11 @@ public CompletableFuture createComment(final int number, final String b */ public CompletableFuture editComment(final int id, final String body) { final String path = String.format(COMMENTS_URI_ID_TEMPLATE, owner, repo, id); - return github + CompletableFuture future = github .patch(path, github.json().toJsonUnchecked(ImmutableMap.of("body", body))) .thenAccept(IGNORE_RESPONSE_CONSUMER); + tracer.span("Edit comment", future); + return future; } /** @@ -117,9 +130,11 @@ public CompletableFuture editComment(final int id, final String body) { * @param id comment id */ public CompletableFuture deleteComment(final int id) { - return github + CompletableFuture future = github .delete(String.format(COMMENTS_URI_ID_TEMPLATE, owner, repo, id)) .thenAccept(IGNORE_RESPONSE_CONSUMER); + tracer.span("Delete comment", future); + return future; } private Iterator> listComments(final String path) { diff --git a/src/main/java/com/spotify/github/v3/clients/NoopTracer.java b/src/main/java/com/spotify/github/v3/clients/NoopTracer.java new file mode 100644 index 00000000..697201cf --- /dev/null +++ b/src/main/java/com/spotify/github/v3/clients/NoopTracer.java @@ -0,0 +1,52 @@ +/* + * Copyright (c) 2019 Spotify AB + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not + * use this file except in compliance with the License. You may obtain a copy of + * the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + */ + +package com.spotify.github.v3.clients; +import com.spotify.github.Span; +import com.spotify.github.Tracer; + +import java.util.concurrent.CompletionStage; + +public class NoopTracer implements Tracer { + + public static final NoopTracer INSTANCE = new NoopTracer(); + private static final Span SPAN = + new Span() { + @Override + public Span success() { + return this; + } + + @Override + public Span failure() { + return this; + } + + @Override + public void close() {} + }; + + private NoopTracer() {} + + @Override + public Span span( + final String name, + final CompletionStage future) { + return SPAN; + } + +} + diff --git a/src/main/java/com/spotify/github/v3/clients/PullRequestClient.java b/src/main/java/com/spotify/github/v3/clients/PullRequestClient.java index 00ed9974..fd33661b 100644 --- a/src/main/java/com/spotify/github/v3/clients/PullRequestClient.java +++ b/src/main/java/com/spotify/github/v3/clients/PullRequestClient.java @@ -27,6 +27,7 @@ import static com.spotify.github.v3.clients.GitHubClient.LIST_REVIEW_TYPE_REFERENCE; import com.google.common.base.Strings; +import com.spotify.github.Tracer; import com.spotify.github.async.AsyncPage; import com.spotify.github.v3.prs.*; import com.spotify.github.v3.prs.requests.PullRequestCreate; @@ -53,6 +54,7 @@ public class PullRequestClient { private final GitHubClient github; private final String owner; private final String repo; + private Tracer tracer = NoopTracer.INSTANCE; PullRequestClient(final GitHubClient github, final String owner, final String repo) { this.github = github; @@ -65,6 +67,11 @@ static PullRequestClient create( return new PullRequestClient(github, owner, repo); } + public PullRequestClient withTracer(Tracer tracer) { + this.tracer = tracer; + return this; + } + /** * List repository pull request. * @@ -95,7 +102,9 @@ public CompletableFuture> list(final PullRequestParameters public CompletableFuture get(final int number) { final String path = String.format(PR_NUMBER_TEMPLATE, owner, repo, number); log.debug("Fetching pull request from " + path); - return github.request(path, PullRequest.class); + CompletableFuture future = github.request(path, PullRequest.class); + tracer.span("Get a specific pull request", future); + return future; } /** @@ -105,9 +114,11 @@ public CompletableFuture get(final int number) { */ public CompletableFuture create(final PullRequestCreate request) { final String path = String.format(PR_TEMPLATE, owner, repo); - return github + CompletableFuture future = github .post(path, github.json().toJsonUnchecked(request)) .thenAccept(IGNORE_RESPONSE_CONSUMER); + tracer.span("Create a pull request", future); + return future; } /** @@ -118,9 +129,11 @@ public CompletableFuture create(final PullRequestCreate request) { */ public CompletableFuture update(final int number, final PullRequestUpdate request) { final String path = String.format(PR_NUMBER_TEMPLATE, owner, repo, number); - return github + CompletableFuture future = github .patch(path, github.json().toJsonUnchecked(request)) .thenAccept(IGNORE_RESPONSE_CONSUMER); + tracer.span("Update given pull request", future); + return future; } /** @@ -132,7 +145,9 @@ public CompletableFuture update(final int number, final PullRequestUpdate public CompletableFuture> listCommits(final int number) { final String path = String.format(PR_COMMITS_TEMPLATE, owner, repo, number); log.debug("Fetching pull request commits from " + path); - return github.request(path, LIST_COMMIT_TYPE_REFERENCE); + CompletableFuture> future = github.request(path, LIST_COMMIT_TYPE_REFERENCE); + tracer.span("List pull request commits", future); + return future; } /** @@ -144,7 +159,9 @@ public CompletableFuture> listCommits(final int number) { public CompletableFuture> listReviews(final int number) { final String path = String.format(PR_REVIEWS_TEMPLATE, owner, repo, number); log.debug("Fetching pull request reviews from " + path); - return github.request(path, LIST_REVIEW_TYPE_REFERENCE); + CompletableFuture> future = github.request(path, LIST_REVIEW_TYPE_REFERENCE); + tracer.span("List pull request reviews", future); + return future; } /** @@ -172,7 +189,9 @@ public CompletableFuture createReview(final int number, final ReviewPara final String path = String.format(PR_REVIEWS_TEMPLATE, owner, repo, number); final String jsonPayload = github.json().toJsonUnchecked(properties); log.debug("Creating review for PR: " + path); - return github.post(path, jsonPayload, Review.class); + CompletableFuture future = github.post(path, jsonPayload, Review.class); + tracer.span("Create a review for a pull request", future); + return future; } /** @@ -184,7 +203,9 @@ public CompletableFuture createReview(final int number, final ReviewPara public CompletableFuture listReviewRequests(final int number) { final String path = String.format(PR_REVIEW_REQUESTS_TEMPLATE, owner, repo, number); log.debug("Fetching pull request requested reviews from " + path); - return github.request(path, LIST_REVIEW_REQUEST_TYPE_REFERENCE); + CompletableFuture future = github.request(path, LIST_REVIEW_REQUEST_TYPE_REFERENCE); + tracer.span("List pull request requested reviews", future); + return future; } /** @@ -198,7 +219,9 @@ public CompletableFuture requestReview(final int number, final Requ final String path = String.format(PR_REVIEW_REQUESTS_TEMPLATE, owner, repo, number); final String jsonPayload = github.json().toJsonUnchecked(properties); log.debug("Requesting reviews for PR: " + path); - return github.post(path, jsonPayload, PullRequest.class); + CompletableFuture future = github.post(path, jsonPayload, PullRequest.class); + tracer.span("Request review for a pull request", future); + return future; } /** @@ -212,7 +235,9 @@ public CompletableFuture removeRequestedReview(final int number, final Req final String path = String.format(PR_REVIEW_REQUESTS_TEMPLATE, owner, repo, number); final String jsonPayload = github.json().toJsonUnchecked(properties); log.debug("Removing requested reviews for PR: " + path); - return github.delete(path, jsonPayload).thenAccept(IGNORE_RESPONSE_CONSUMER); + CompletableFuture future = github.delete(path, jsonPayload).thenAccept(IGNORE_RESPONSE_CONSUMER); + tracer.span("Remove request for review for a pull request", future); + return future; } /** @@ -226,13 +251,17 @@ public CompletableFuture merge(final int number, final MergeParameters pro final String path = String.format(PR_NUMBER_TEMPLATE + "/merge", owner, repo, number); final String jsonPayload = github.json().toJsonUnchecked(properties); log.debug("Merging pr, running: {}", path); - return github.put(path, jsonPayload).thenAccept(IGNORE_RESPONSE_CONSUMER); + CompletableFuture future = github.put(path, jsonPayload).thenAccept(IGNORE_RESPONSE_CONSUMER); + tracer.span("Merge pull request", future); + return future; } private CompletableFuture> list(final String parameterPath) { final String path = String.format(PR_TEMPLATE + parameterPath, owner, repo); log.debug("Fetching pull requests from " + path); - return github.request(path, LIST_PR_TYPE_REFERENCE); + CompletableFuture> future = github.request(path, LIST_PR_TYPE_REFERENCE); + tracer.span("List pull requests", future); + return future; } } diff --git a/src/main/java/com/spotify/github/v3/clients/RepositoryClient.java b/src/main/java/com/spotify/github/v3/clients/RepositoryClient.java index fd806256..0958a11c 100644 --- a/src/main/java/com/spotify/github/v3/clients/RepositoryClient.java +++ b/src/main/java/com/spotify/github/v3/clients/RepositoryClient.java @@ -26,9 +26,11 @@ import static com.spotify.github.v3.clients.GitHubClient.LIST_FOLDERCONTENT_TYPE_REFERENCE; import static com.spotify.github.v3.clients.GitHubClient.LIST_REPOSITORY; import static com.spotify.github.v3.clients.GitHubClient.LIST_STATUS_TYPE_REFERENCE; +import static java.util.Objects.requireNonNull; import com.google.common.base.Strings; import com.google.common.collect.ImmutableMap; +import com.spotify.github.Tracer; import com.spotify.github.async.AsyncPage; import com.spotify.github.v3.comment.Comment; import com.spotify.github.v3.exceptions.RequestNotOkException; @@ -85,6 +87,7 @@ public class RepositoryClient { private final String owner; private final String repo; private final GitHubClient github; + private Tracer tracer = NoopTracer.INSTANCE; RepositoryClient(final GitHubClient github, final String owner, final String repo) { this.github = github; @@ -96,6 +99,11 @@ static RepositoryClient create(final GitHubClient github, final String owner, fi return new RepositoryClient(github, owner, repo); } + public RepositoryClient withTracer(Tracer tracer) { + this.tracer = requireNonNull(tracer); + return this; + } + /** * Create an issue API client. * @@ -142,7 +150,9 @@ public ChecksClient createChecksApiClient() { */ public CompletableFuture getRepository() { final String path = String.format(REPOSITORY_URI_TEMPLATE, owner, repo); - return github.request(path, Repository.class); + CompletableFuture future = github.request(path, Repository.class); + tracer.span("Get repository", future); + return future; } /** @@ -152,7 +162,9 @@ public CompletableFuture getRepository() { */ public CompletableFuture> listOrganizationRepositories() { final String path = String.format(LIST_REPOSITORY_TEMPLATE, owner); - return github.request(path, LIST_REPOSITORY); + CompletableFuture> future = github.request(path, LIST_REPOSITORY); + tracer.span("List repositories", future); + return future; } /** @@ -178,7 +190,9 @@ public Iterator> listAuthenticatedUserRepositories( */ public CompletableFuture isCollaborator(final String user) { final String path = String.format(IS_USER_COLLABORATOR_OF_REPO, owner, repo, user); - return github.request(path).thenApply(response -> response.code() == NO_CONTENT); + CompletableFuture future = github.request(path).thenApply(response -> response.code() == NO_CONTENT); + tracer.span("isCollaborator", future); + return future; } /** @@ -190,8 +204,7 @@ public CompletableFuture isCollaborator(final String user) { public CompletableFuture createWebhook( final WebhookCreate request, final boolean ignoreExisting) { final String path = String.format(HOOK_URI_TEMPLATE, owner, repo); - - return github + CompletableFuture future = github .post(path, github.json().toJsonUnchecked(request)) .thenAccept(IGNORE_RESPONSE_CONSUMER) .exceptionally( @@ -211,6 +224,8 @@ public CompletableFuture createWebhook( throw new CompletionException(e); }); + tracer.span("Create webhook", future); + return future; } /** @@ -222,9 +237,11 @@ public CompletableFuture createWebhook( public CompletableFuture setCommitStatus( final String sha, final RepositoryCreateStatus request) { final String path = String.format(STATUS_URI_TEMPLATE, owner, repo, sha); - return github + CompletableFuture future = github .post(path, github.json().toJsonUnchecked(request)) .thenAccept(IGNORE_RESPONSE_CONSUMER); + tracer.span("Set commit status", future); + return future; } /** @@ -234,7 +251,9 @@ public CompletableFuture setCommitStatus( */ public CompletableFuture getCommitStatus(final String ref) { final String path = String.format(COMMIT_STATUS_URI_TEMPLATE, owner, repo, ref); - return github.request(path, CommitStatus.class); + CompletableFuture future = github.request(path, CommitStatus.class); + tracer.span("Get commit status", future); + return future; } /** @@ -245,7 +264,9 @@ public CompletableFuture getCommitStatus(final String ref) { */ public CompletableFuture> listCommitStatuses(final String sha) { final String path = String.format(STATUS_URI_TEMPLATE, owner, repo, sha); - return github.request(path, LIST_STATUS_TYPE_REFERENCE); + CompletableFuture> future = github.request(path, LIST_STATUS_TYPE_REFERENCE); + tracer.span("List commit statuses", future); + return future; } /** @@ -270,7 +291,9 @@ public Iterator> listCommitStatuses(final String sha, final in */ public CompletableFuture> listCommits() { final String path = String.format(COMMITS_URI_TEMPLATE, owner, repo); - return github.request(path, LIST_COMMIT_TYPE_REFERENCE); + CompletableFuture> future = github.request(path, LIST_COMMIT_TYPE_REFERENCE); + tracer.span("List commits", future); + return future; } /** @@ -281,7 +304,9 @@ public CompletableFuture> listCommits() { */ public CompletableFuture getCommit(final String sha) { final String path = String.format(COMMIT_SHA_URI_TEMPLATE, owner, repo, sha); - return github.request(path, Commit.class); + CompletableFuture future = github.request(path, Commit.class); + tracer.span("Get commit", future); + return future; } /** @@ -294,7 +319,9 @@ public CompletableFuture getCommit(final String sha) { @Deprecated public CompletableFuture getTree(final String sha) { final String path = String.format(TREE_SHA_URI_TEMPLATE, owner, repo, sha); - return github.request(path, Tree.class); + CompletableFuture future = github.request(path, Tree.class); + tracer.span("Get tree", future); + return future; } /** @@ -304,7 +331,9 @@ public CompletableFuture getTree(final String sha) { * @return content */ public CompletableFuture getFileContent(final String path) { - return github.request(getContentPath(path, ""), Content.class); + CompletableFuture future = github.request(getContentPath(path, ""), Content.class); + tracer.span("Get file content", future); + return future; } /** @@ -315,7 +344,9 @@ public CompletableFuture getFileContent(final String path) { * @return content */ public CompletableFuture getFileContent(final String path, final String ref) { - return github.request(getContentPath(path, "?ref=" + ref), Content.class); + CompletableFuture future = github.request(getContentPath(path, "?ref=" + ref), Content.class); + tracer.span("Get file content", future); + return future; } /** @@ -325,7 +356,9 @@ public CompletableFuture getFileContent(final String path, final String * @return content */ public CompletableFuture> getFolderContent(final String path) { - return github.request(getContentPath(path, ""), LIST_FOLDERCONTENT_TYPE_REFERENCE); + CompletableFuture> future = github.request(getContentPath(path, ""), LIST_FOLDERCONTENT_TYPE_REFERENCE); + tracer.span("Get folder content", future); + return future; } /** @@ -338,7 +371,9 @@ public CompletableFuture> getFolderContent(final String path public CompletableFuture createComment(final String sha, final String body) { final String path = String.format(CREATE_COMMENT_TEMPLATE, owner, repo, sha); final String requestBody = github.json().toJsonUnchecked(ImmutableMap.of("body", body)); - return github.post(path, requestBody, Comment.class); + CompletableFuture future = github.post(path, requestBody, Comment.class); + tracer.span("Create comment", future); + return future; } /** @@ -349,7 +384,9 @@ public CompletableFuture createComment(final String sha, final String b */ public CompletableFuture getComment(final int id) { final String path = String.format(COMMENT_TEMPLATE, owner, repo, id); - return github.request(path, Comment.class); + CompletableFuture future = github.request(path, Comment.class); + tracer.span("Get comment", future); + return future; } /** @@ -361,7 +398,9 @@ public CompletableFuture getComment(final int id) { */ public CompletableFuture> getFolderContent( final String path, final String ref) { - return github.request(getContentPath(path, "?ref=" + ref), LIST_FOLDERCONTENT_TYPE_REFERENCE); + CompletableFuture> future = github.request(getContentPath(path, "?ref=" + ref), LIST_FOLDERCONTENT_TYPE_REFERENCE); + tracer.span("Get folder content", future); + return future; } /** @@ -373,7 +412,9 @@ public CompletableFuture> getFolderContent( */ public CompletableFuture compareCommits(final String base, final String head) { final String path = String.format(COMPARE_COMMIT_TEMPLATE, owner, repo, base, head); - return github.request(path, CommitComparison.class); + CompletableFuture future = github.request(path, CommitComparison.class); + tracer.span("Compare commits", future); + return future; } /** @@ -384,7 +425,9 @@ public CompletableFuture compareCommits(final String base, fin */ public CompletableFuture getBranch(final String branch) { final String path = String.format(BRANCH_TEMPLATE, owner, repo, branch); - return github.request(path, Branch.class); + CompletableFuture future = github.request(path, Branch.class); + tracer.span("Get branch", future); + return future; } /** @@ -394,7 +437,9 @@ public CompletableFuture getBranch(final String branch) { */ public CompletableFuture> listBranches() { final String path = String.format(LIST_BRANCHES_TEMPLATE, owner, repo); - return github.request(path, LIST_BRANCHES); + CompletableFuture> future = github.request(path, LIST_BRANCHES); + tracer.span("List branches", future); + return future; } /** @@ -404,7 +449,9 @@ public CompletableFuture> listBranches() { */ public CompletableFuture deleteComment(final int id) { final String path = String.format(COMMENT_TEMPLATE, owner, repo, id); - return github.delete(path).thenAccept(IGNORE_RESPONSE_CONSUMER); + CompletableFuture future = github.delete(path).thenAccept(IGNORE_RESPONSE_CONSUMER); + tracer.span("Delete comment", future); + return future; } /** @@ -415,9 +462,11 @@ public CompletableFuture deleteComment(final int id) { */ public CompletableFuture editComment(final int id, final String body) { final String path = String.format(COMMENT_TEMPLATE, owner, repo, id); - return github + CompletableFuture future = github .patch(path, github.json().toJsonUnchecked(ImmutableMap.of("body", body))) .thenAccept(IGNORE_RESPONSE_CONSUMER); + tracer.span("Edit comment", future); + return future; } /** @@ -427,7 +476,9 @@ public CompletableFuture editComment(final int id, final String body) { */ public CompletableFuture getLanguages() { final String path = String.format(LANGUAGES_TEMPLATE, owner, repo); - return github.request(path, Languages.class); + CompletableFuture future = github.request(path, Languages.class); + tracer.span("Get languages", future); + return future; } /** @@ -439,7 +490,9 @@ public CompletableFuture getLanguages() { * @return resulting merge commit, or empty if base already contains the head (nothing to merge) */ public CompletableFuture> merge(final String base, final String head) { - return merge(base, head, null); + CompletableFuture> future = merge(base, head, null); + tracer.span("Merge", future); + return future; } /** @@ -460,7 +513,7 @@ public CompletableFuture> merge( : ImmutableMap.of("base", base, "head", head, "commit_message", commitMessage); final String body = github.json().toJsonUnchecked(params); - return github + CompletableFuture> future = github .post(path, body) .thenApply( response -> { @@ -478,6 +531,8 @@ public CompletableFuture> merge( GitHubClient.responseBodyUnchecked(response), CommitItem.class); return Optional.of(commitItem); }); + tracer.span("Merge", future); + return future; } /** @@ -493,7 +548,7 @@ public CompletableFuture createFork(final String organization) { (organization == null) ? ImmutableMap.of() : ImmutableMap.of("organization", organization); final String body = github.json().toJsonUnchecked(params); - return github + CompletableFuture future = github .post(path, body) .thenApply( response -> { @@ -504,6 +559,8 @@ public CompletableFuture createFork(final String organization) { GitHubClient.responseBodyUnchecked(response), Repository.class); return repositoryItem; }); + tracer.span("Create fork", future); + return future; } private String getContentPath(final String path, final String query) { diff --git a/src/main/java/com/spotify/github/v3/clients/SearchClient.java b/src/main/java/com/spotify/github/v3/clients/SearchClient.java index 764a9657..d7752155 100644 --- a/src/main/java/com/spotify/github/v3/clients/SearchClient.java +++ b/src/main/java/com/spotify/github/v3/clients/SearchClient.java @@ -21,6 +21,7 @@ package com.spotify.github.v3.clients; import com.google.common.base.Strings; +import com.spotify.github.Tracer; import com.spotify.github.v3.search.SearchIssues; import com.spotify.github.v3.search.SearchRepositories; import com.spotify.github.v3.search.SearchUsers; @@ -38,6 +39,7 @@ public class SearchClient { static final String REPOSITORIES_URI = "/search/repositories"; static final String ISSUES_URI = "/search/issues"; private final GitHubClient github; + private Tracer tracer = NoopTracer.INSTANCE; SearchClient(final GitHubClient github) { this.github = github; @@ -47,6 +49,10 @@ static SearchClient create(final GitHubClient github) { return new SearchClient(github); } + public SearchClient withTracer(Tracer tracer) { + this.tracer = tracer; + return this; + } /** * Search users. * @@ -54,7 +60,9 @@ static SearchClient create(final GitHubClient github) { * @return user search results */ public CompletableFuture users(final SearchParameters parameters) { - return search(USERS_URI, parameters, SearchUsers.class); + CompletableFuture future = search(USERS_URI, parameters, SearchUsers.class); + tracer.span("Search users", future); + return future; } /** @@ -64,7 +72,9 @@ public CompletableFuture users(final SearchParameters parameters) { * @return issue search results */ public CompletableFuture issues(final SearchParameters parameters) { - return search(ISSUES_URI, parameters, SearchIssues.class); + CompletableFuture future = search(ISSUES_URI, parameters, SearchIssues.class); + tracer.span("Search issues", future); + return future; } /** @@ -74,7 +84,9 @@ public CompletableFuture issues(final SearchParameters parameters) * @return repository search results */ public CompletableFuture repositories(final SearchParameters parameters) { - return search(REPOSITORIES_URI, parameters, SearchRepositories.class); + CompletableFuture future = search(REPOSITORIES_URI, parameters, SearchRepositories.class); + tracer.span("Search repositories", future); + return future; } private CompletableFuture search( From 4310e5c778e15d87fafd05eb91d51554b03f26a8 Mon Sep 17 00:00:00 2001 From: Dennis Granath Date: Fri, 15 Oct 2021 15:03:49 +0200 Subject: [PATCH 2/7] Add opencensus tracer --- pom.xml | 6 ++ src/main/java/com/spotify/github/Span.java | 5 +- src/main/java/com/spotify/github/Tracer.java | 7 +- .../github/opencensus/OpenCensusSpan.java | 50 ++++++++++++++ .../github/opencensus/OpenCensusTracer.java | 66 +++++++++++++++++++ .../github/v3/clients/ChecksClient.java | 2 +- .../github/v3/clients/GitDataClient.java | 2 +- .../github/v3/clients/GithubAppClient.java | 2 +- .../github/v3/clients/IssueClient.java | 2 +- .../spotify/github/v3/clients/NoopTracer.java | 5 +- .../github/v3/clients/PullRequestClient.java | 2 +- .../github/v3/clients/RepositoryClient.java | 10 +-- .../github/v3/clients/SearchClient.java | 2 +- 13 files changed, 146 insertions(+), 15 deletions(-) create mode 100644 src/main/java/com/spotify/github/opencensus/OpenCensusSpan.java create mode 100644 src/main/java/com/spotify/github/opencensus/OpenCensusTracer.java diff --git a/pom.xml b/pom.xml index 00147ac8..dfc7e59b 100644 --- a/pom.xml +++ b/pom.xml @@ -100,6 +100,7 @@ 2.8.3 2.0.2 3.0.1 + 0.22.1 ${project.groupId}.githubclient.shade @@ -170,6 +171,11 @@ jjwt-api 0.10.5 + + io.opencensus + opencensus-api + ${opencensus.version} + io.jsonwebtoken jjwt-impl diff --git a/src/main/java/com/spotify/github/Span.java b/src/main/java/com/spotify/github/Span.java index 0cd62565..fde10541 100644 --- a/src/main/java/com/spotify/github/Span.java +++ b/src/main/java/com/spotify/github/Span.java @@ -1,5 +1,8 @@ /* - * Copyright (c) 2019 Spotify AB + * -\-\- + * github-client + * -- + * Copyright (c) 2021 Spotify AB * * Licensed under the Apache License, Version 2.0 (the "License"); you may not * use this file except in compliance with the License. You may obtain a copy of diff --git a/src/main/java/com/spotify/github/Tracer.java b/src/main/java/com/spotify/github/Tracer.java index cbff07c3..36d63d7c 100644 --- a/src/main/java/com/spotify/github/Tracer.java +++ b/src/main/java/com/spotify/github/Tracer.java @@ -1,5 +1,8 @@ /* - * Copyright (c) 2019 Spotify AB + * -\-\- + * github-client + * -- + * Copyright (c) 2021 Spotify AB * * Licensed under the Apache License, Version 2.0 (the "License"); you may not * use this file except in compliance with the License. You may obtain a copy of @@ -22,7 +25,7 @@ public interface Tracer { /** Create scoped span. Span will be closed when future completes. */ Span span( - final String name, final CompletionStage future); + String name, CompletionStage future); } diff --git a/src/main/java/com/spotify/github/opencensus/OpenCensusSpan.java b/src/main/java/com/spotify/github/opencensus/OpenCensusSpan.java new file mode 100644 index 00000000..c3186c76 --- /dev/null +++ b/src/main/java/com/spotify/github/opencensus/OpenCensusSpan.java @@ -0,0 +1,50 @@ +/* + * -\-\- + * github-client + * -- + * Copyright (c) 2019 Spotify AB + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not + * use this file except in compliance with the License. You may obtain a copy of + * the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + */ + +package com.spotify.github.opencensus; +import static java.util.Objects.requireNonNull; +import com.spotify.github.Span; +import io.opencensus.trace.Status; + +class OpenCensusSpan implements Span { + + private final io.opencensus.trace.Span span; + + OpenCensusSpan(final io.opencensus.trace.Span span) { + this.span = requireNonNull(span); + } + + @Override + public Span success() { + span.setStatus(Status.OK); + return this; + } + + @Override + public Span failure() { + span.setStatus(Status.UNKNOWN); + return this; + } + + @Override + public void close() { + span.end(); + } +} + diff --git a/src/main/java/com/spotify/github/opencensus/OpenCensusTracer.java b/src/main/java/com/spotify/github/opencensus/OpenCensusTracer.java new file mode 100644 index 00000000..d351feca --- /dev/null +++ b/src/main/java/com/spotify/github/opencensus/OpenCensusTracer.java @@ -0,0 +1,66 @@ +/* + * -\-\- + * github-client + * -- + * Copyright (c) 2021 Spotify AB + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not + * use this file except in compliance with the License. You may obtain a copy of + * the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + */ +package com.spotify.github.opencensus; + +import com.spotify.github.Span; +import com.spotify.github.Tracer; +import io.opencensus.trace.Tracing; + +import java.util.concurrent.CompletionStage; + +import static io.opencensus.trace.AttributeValue.stringAttributeValue; +import static io.opencensus.trace.Span.Kind.CLIENT; +import static java.util.Objects.requireNonNull; + +public class OpenCensusTracer implements Tracer { + + private static final io.opencensus.trace.Tracer TRACER = Tracing.getTracer(); + + @Override + public Span span(final String name, final CompletionStage future) { + return internalSpan(name, future); + } + + @SuppressWarnings("MustBeClosedChecker") + private Span internalSpan( + final String name, + final CompletionStage future) { + requireNonNull(name); + requireNonNull(future); + + final io.opencensus.trace.Span ocSpan = + TRACER.spanBuilder(name).setSpanKind(CLIENT).startSpan(); + + ocSpan.putAttribute("component", stringAttributeValue("github-api-client")); + ocSpan.putAttribute("peer.service", stringAttributeValue("github")); + final Span span = new OpenCensusSpan(ocSpan); + + future.whenComplete( + (result, t) -> { + if (t == null) { + span.success(); + } else { + span.failure(); + } + span.close(); + }); + + return span; + } +} diff --git a/src/main/java/com/spotify/github/v3/clients/ChecksClient.java b/src/main/java/com/spotify/github/v3/clients/ChecksClient.java index ea3d139f..c1a1417c 100644 --- a/src/main/java/com/spotify/github/v3/clients/ChecksClient.java +++ b/src/main/java/com/spotify/github/v3/clients/ChecksClient.java @@ -63,7 +63,7 @@ public class ChecksClient { this.repo = repo; } - public ChecksClient withTracer(Tracer tracer) { + public ChecksClient withTracer(final Tracer tracer) { this.tracer = tracer; return this; } diff --git a/src/main/java/com/spotify/github/v3/clients/GitDataClient.java b/src/main/java/com/spotify/github/v3/clients/GitDataClient.java index 6729be78..97587ef4 100644 --- a/src/main/java/com/spotify/github/v3/clients/GitDataClient.java +++ b/src/main/java/com/spotify/github/v3/clients/GitDataClient.java @@ -70,7 +70,7 @@ static GitDataClient create(final GitHubClient github, final String owner, final return new GitDataClient(github, owner, repo); } - public GitDataClient withTracer(Tracer tracer) { + public GitDataClient withTracer(final Tracer tracer) { this.tracer = tracer; return this; } diff --git a/src/main/java/com/spotify/github/v3/clients/GithubAppClient.java b/src/main/java/com/spotify/github/v3/clients/GithubAppClient.java index 1992ee5c..b639e025 100644 --- a/src/main/java/com/spotify/github/v3/clients/GithubAppClient.java +++ b/src/main/java/com/spotify/github/v3/clients/GithubAppClient.java @@ -56,7 +56,7 @@ public class GithubAppClient { this.repo = repo; } - public GithubAppClient withTracer(Tracer tracer) { + public GithubAppClient withTracer(final Tracer tracer) { this.tracer = tracer; return this; } diff --git a/src/main/java/com/spotify/github/v3/clients/IssueClient.java b/src/main/java/com/spotify/github/v3/clients/IssueClient.java index 8ef6f684..2352e32e 100644 --- a/src/main/java/com/spotify/github/v3/clients/IssueClient.java +++ b/src/main/java/com/spotify/github/v3/clients/IssueClient.java @@ -56,7 +56,7 @@ static IssueClient create(final GitHubClient github, final String owner, final S return new IssueClient(github, owner, repo); } - public IssueClient withTracer(Tracer tracer) { + public IssueClient withTracer(final Tracer tracer) { this.tracer = tracer; return this; } diff --git a/src/main/java/com/spotify/github/v3/clients/NoopTracer.java b/src/main/java/com/spotify/github/v3/clients/NoopTracer.java index 697201cf..c3cd4f68 100644 --- a/src/main/java/com/spotify/github/v3/clients/NoopTracer.java +++ b/src/main/java/com/spotify/github/v3/clients/NoopTracer.java @@ -1,5 +1,8 @@ /* - * Copyright (c) 2019 Spotify AB + * -\-\- + * github-client + * -- + * Copyright (c) 2021 Spotify AB * * Licensed under the Apache License, Version 2.0 (the "License"); you may not * use this file except in compliance with the License. You may obtain a copy of diff --git a/src/main/java/com/spotify/github/v3/clients/PullRequestClient.java b/src/main/java/com/spotify/github/v3/clients/PullRequestClient.java index fd33661b..75d2536d 100644 --- a/src/main/java/com/spotify/github/v3/clients/PullRequestClient.java +++ b/src/main/java/com/spotify/github/v3/clients/PullRequestClient.java @@ -67,7 +67,7 @@ static PullRequestClient create( return new PullRequestClient(github, owner, repo); } - public PullRequestClient withTracer(Tracer tracer) { + public PullRequestClient withTracer(final Tracer tracer) { this.tracer = tracer; return this; } diff --git a/src/main/java/com/spotify/github/v3/clients/RepositoryClient.java b/src/main/java/com/spotify/github/v3/clients/RepositoryClient.java index 0958a11c..a5c3a287 100644 --- a/src/main/java/com/spotify/github/v3/clients/RepositoryClient.java +++ b/src/main/java/com/spotify/github/v3/clients/RepositoryClient.java @@ -99,7 +99,7 @@ static RepositoryClient create(final GitHubClient github, final String owner, fi return new RepositoryClient(github, owner, repo); } - public RepositoryClient withTracer(Tracer tracer) { + public RepositoryClient withTracer(final Tracer tracer) { this.tracer = requireNonNull(tracer); return this; } @@ -110,7 +110,7 @@ public RepositoryClient withTracer(Tracer tracer) { * @return issue API client */ public IssueClient createIssueClient() { - return IssueClient.create(github, owner, repo); + return IssueClient.create(github, owner, repo).withTracer(tracer); } /** @@ -119,7 +119,7 @@ public IssueClient createIssueClient() { * @return pull request API client */ public PullRequestClient createPullRequestClient() { - return PullRequestClient.create(github, owner, repo); + return PullRequestClient.create(github, owner, repo).withTracer(tracer); } /** @@ -128,7 +128,7 @@ public PullRequestClient createPullRequestClient() { * @return Github App API client */ public GithubAppClient createGithubAppClient() { - return new GithubAppClient(github, owner, repo); + return new GithubAppClient(github, owner, repo).withTracer(tracer); } /** @@ -140,7 +140,7 @@ public ChecksClient createChecksApiClient() { if (!github.getPrivateKey().isPresent()) { throw new IllegalArgumentException("Checks Client needs a private key"); } - return new ChecksClient(github, owner, repo); + return new ChecksClient(github, owner, repo).withTracer(tracer); } /** diff --git a/src/main/java/com/spotify/github/v3/clients/SearchClient.java b/src/main/java/com/spotify/github/v3/clients/SearchClient.java index d7752155..7f2fb59f 100644 --- a/src/main/java/com/spotify/github/v3/clients/SearchClient.java +++ b/src/main/java/com/spotify/github/v3/clients/SearchClient.java @@ -49,7 +49,7 @@ static SearchClient create(final GitHubClient github) { return new SearchClient(github); } - public SearchClient withTracer(Tracer tracer) { + public SearchClient withTracer(final Tracer tracer) { this.tracer = tracer; return this; } From 96496fd61bee26348d5b853bc69376939aadd84f Mon Sep 17 00:00:00 2001 From: Dennis Granath Date: Mon, 18 Oct 2021 12:48:40 +0200 Subject: [PATCH 3/7] Unit test OpenCensusSpan --- src/main/java/com/spotify/github/Span.java | 29 ++++++----- src/main/java/com/spotify/github/Tracer.java | 29 ++++++----- .../github/opencensus/OpenCensusSpan.java | 29 ++++++----- .../github/opencensus/OpenCensusTracer.java | 30 ++++++----- .../spotify/github/v3/clients/NoopTracer.java | 29 ++++++----- .../github/opencensus/OpenCensusSpanTest.java | 52 +++++++++++++++++++ 6 files changed, 128 insertions(+), 70 deletions(-) create mode 100644 src/test/java/com/spotify/github/opencensus/OpenCensusSpanTest.java diff --git a/src/main/java/com/spotify/github/Span.java b/src/main/java/com/spotify/github/Span.java index fde10541..f8a24aeb 100644 --- a/src/main/java/com/spotify/github/Span.java +++ b/src/main/java/com/spotify/github/Span.java @@ -1,20 +1,21 @@ -/* +/*- * -\-\- - * github-client + * github-api * -- - * Copyright (c) 2021 Spotify AB - * - * Licensed under the Apache License, Version 2.0 (the "License"); you may not - * use this file except in compliance with the License. You may obtain a copy of - * the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * + * Copyright (C) 2021 Spotify AB + * -- + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT - * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the - * License for the specific language governing permissions and limitations under - * the License. + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * -/-/- */ package com.spotify.github; diff --git a/src/main/java/com/spotify/github/Tracer.java b/src/main/java/com/spotify/github/Tracer.java index 36d63d7c..d7a9c3e2 100644 --- a/src/main/java/com/spotify/github/Tracer.java +++ b/src/main/java/com/spotify/github/Tracer.java @@ -1,20 +1,21 @@ -/* +/*- * -\-\- - * github-client + * github-api * -- - * Copyright (c) 2021 Spotify AB - * - * Licensed under the Apache License, Version 2.0 (the "License"); you may not - * use this file except in compliance with the License. You may obtain a copy of - * the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * + * Copyright (C) 2021 Spotify AB + * -- + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT - * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the - * License for the specific language governing permissions and limitations under - * the License. + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * -/-/- */ package com.spotify.github; diff --git a/src/main/java/com/spotify/github/opencensus/OpenCensusSpan.java b/src/main/java/com/spotify/github/opencensus/OpenCensusSpan.java index c3186c76..9b8a4872 100644 --- a/src/main/java/com/spotify/github/opencensus/OpenCensusSpan.java +++ b/src/main/java/com/spotify/github/opencensus/OpenCensusSpan.java @@ -1,20 +1,21 @@ -/* +/*- * -\-\- - * github-client + * github-api * -- - * Copyright (c) 2019 Spotify AB - * - * Licensed under the Apache License, Version 2.0 (the "License"); you may not - * use this file except in compliance with the License. You may obtain a copy of - * the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * + * Copyright (C) 2016 - 2021 Spotify AB + * -- + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT - * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the - * License for the specific language governing permissions and limitations under - * the License. + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * -/-/- */ package com.spotify.github.opencensus; diff --git a/src/main/java/com/spotify/github/opencensus/OpenCensusTracer.java b/src/main/java/com/spotify/github/opencensus/OpenCensusTracer.java index d351feca..bf1d30e1 100644 --- a/src/main/java/com/spotify/github/opencensus/OpenCensusTracer.java +++ b/src/main/java/com/spotify/github/opencensus/OpenCensusTracer.java @@ -1,21 +1,23 @@ -/* +/*- * -\-\- - * github-client + * github-api * -- - * Copyright (c) 2021 Spotify AB - * - * Licensed under the Apache License, Version 2.0 (the "License"); you may not - * use this file except in compliance with the License. You may obtain a copy of - * the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * + * Copyright (C) 2021 Spotify AB + * -- + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT - * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the - * License for the specific language governing permissions and limitations under - * the License. + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * -/-/- */ + package com.spotify.github.opencensus; import com.spotify.github.Span; diff --git a/src/main/java/com/spotify/github/v3/clients/NoopTracer.java b/src/main/java/com/spotify/github/v3/clients/NoopTracer.java index c3cd4f68..16493933 100644 --- a/src/main/java/com/spotify/github/v3/clients/NoopTracer.java +++ b/src/main/java/com/spotify/github/v3/clients/NoopTracer.java @@ -1,20 +1,21 @@ -/* +/*- * -\-\- - * github-client + * github-api * -- - * Copyright (c) 2021 Spotify AB - * - * Licensed under the Apache License, Version 2.0 (the "License"); you may not - * use this file except in compliance with the License. You may obtain a copy of - * the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * + * Copyright (C) 2021 Spotify AB + * -- + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT - * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the - * License for the specific language governing permissions and limitations under - * the License. + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * -/-/- */ package com.spotify.github.v3.clients; diff --git a/src/test/java/com/spotify/github/opencensus/OpenCensusSpanTest.java b/src/test/java/com/spotify/github/opencensus/OpenCensusSpanTest.java new file mode 100644 index 00000000..c3fb3b30 --- /dev/null +++ b/src/test/java/com/spotify/github/opencensus/OpenCensusSpanTest.java @@ -0,0 +1,52 @@ +/*- + * -\-\- + * github-api + * -- + * Copyright (C) 2021 Spotify AB + * -- + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * -/-/- + */ + +package com.spotify.github.opencensus; + +import com.spotify.github.Span; +import io.opencensus.trace.Status; +import org.junit.jupiter.api.Test; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; + +class OpenCensusSpanTest { + private io.opencensus.trace.Span wrapped = mock(io.opencensus.trace.Span.class); + + @Test + public void succeed() { + final Span span = new OpenCensusSpan(wrapped); + span.success(); + span.close(); + + verify(wrapped).setStatus(Status.OK); + verify(wrapped).end(); + } + + @Test + public void fail() { + final Span span = new OpenCensusSpan(wrapped); + span.failure(); + span.close(); + + verify(wrapped).setStatus(Status.UNKNOWN); + verify(wrapped).end(); + } + +} From 15059e95055aed52dc679699cb0f2182ba5eec84 Mon Sep 17 00:00:00 2001 From: Dennis Granath Date: Mon, 18 Oct 2021 13:42:43 +0200 Subject: [PATCH 4/7] Some more tracing tests --- .../spotify/github/opencensus/OpenCensusTracer.java | 1 + .../spotify/github/v3/clients/ChecksClientTest.java | 10 +++++++--- .../github/v3/clients/GitDataClientTest.java | 11 ++++++++--- .../spotify/github/v3/clients/IssueClientTest.java | 8 ++++++-- .../github/v3/clients/PullRequestClientTest.java | 8 ++++---- .../github/v3/clients/RepositoryClientTest.java | 13 ++++++++++--- .../spotify/github/v3/clients/SearchClientTest.java | 9 ++++++--- 7 files changed, 42 insertions(+), 18 deletions(-) diff --git a/src/main/java/com/spotify/github/opencensus/OpenCensusTracer.java b/src/main/java/com/spotify/github/opencensus/OpenCensusTracer.java index bf1d30e1..3a451d78 100644 --- a/src/main/java/com/spotify/github/opencensus/OpenCensusTracer.java +++ b/src/main/java/com/spotify/github/opencensus/OpenCensusTracer.java @@ -55,6 +55,7 @@ private Span internalSpan( future.whenComplete( (result, t) -> { + if (t == null) { span.success(); } else { diff --git a/src/test/java/com/spotify/github/v3/clients/ChecksClientTest.java b/src/test/java/com/spotify/github/v3/clients/ChecksClientTest.java index 46cb2517..93824872 100644 --- a/src/test/java/com/spotify/github/v3/clients/ChecksClientTest.java +++ b/src/test/java/com/spotify/github/v3/clients/ChecksClientTest.java @@ -28,12 +28,14 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.core.Is.is; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; +import static org.mockito.Mockito.*; +import static org.mockito.Mockito.times; import com.google.common.io.Resources; import com.spotify.github.jackson.Json; +import com.spotify.github.opencensus.OpenCensusTracer; import com.spotify.github.v3.checks.CheckRunOutput; import com.spotify.github.v3.checks.CheckRunRequest; import com.spotify.github.v3.checks.CheckRunResponse; @@ -52,6 +54,7 @@ public class ChecksClientTest { private GitHubClient github; private ChecksClient checksClient; private Json json; + private OpenCensusTracer tracer = mock(OpenCensusTracer.class); public static String loadResource(final String path) { try { @@ -64,7 +67,7 @@ public static String loadResource(final String path) { @Before public void setUp() { github = mock(GitHubClient.class); - checksClient = new ChecksClient(github, "someowner", "somerepo"); + checksClient = new ChecksClient(github, "someowner", "somerepo").withTracer(tracer); json = Json.create(); when(github.json()).thenReturn(json); } @@ -90,6 +93,7 @@ public void createCompletedCheckRun() throws Exception { assertThat(actualResponse.get().status(), is(completed)); assertThat(actualResponse.get().headSha(), is("ce587453ced02b1526dfb4cb910479d431683101")); assertThat(actualResponse.get().output().annotationsCount().get(), is(2)); + verify(tracer,times(1)).span(anyString(), any()); } @Test diff --git a/src/test/java/com/spotify/github/v3/clients/GitDataClientTest.java b/src/test/java/com/spotify/github/v3/clients/GitDataClientTest.java index 3874dd74..0f3fbf94 100644 --- a/src/test/java/com/spotify/github/v3/clients/GitDataClientTest.java +++ b/src/test/java/com/spotify/github/v3/clients/GitDataClientTest.java @@ -31,9 +31,10 @@ import static org.hamcrest.core.Is.is; import static org.hamcrest.core.StringContains.containsString; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; +import static org.mockito.Mockito.*; +import static org.mockito.Mockito.times; import com.google.common.collect.ImmutableMap; import com.google.common.io.Resources; @@ -58,6 +59,7 @@ public class GitDataClientTest { private GitHubClient github; private GitDataClient gitDataClient; private Json json; + private NoopTracer tracer = mock(NoopTracer.class); private static String getFixture(String resource) throws IOException { return Resources.toString(getResource(GitDataClientTest.class, resource), defaultCharset()); @@ -66,7 +68,7 @@ private static String getFixture(String resource) throws IOException { @Before public void setUp() { github = mock(GitHubClient.class); - gitDataClient = new GitDataClient(github, "someowner", "somerepo"); + gitDataClient = new GitDataClient(github, "someowner", "somerepo").withTracer(tracer); json = Json.create(); when(github.json()).thenReturn(json); } @@ -79,6 +81,7 @@ public void getTagRef() throws Exception { .thenReturn(fixture); final Reference reference = gitDataClient.getTagReference("0.0.1").get(); assertThat(reference.object().sha(), is("5926dd300de5fee31d445c57be223f00e128a634")); + verify(tracer,times(1)).span(anyString(), any()); } @Test @@ -270,6 +273,8 @@ public void testGetRecursiveTree() throws IOException { .join(); assertThat(tree.sha(), is("9c27bd92524e2b57b569d4c86695b3993d9b8f9f")); assertThat(tree.tree().size(), is(7)); + verify(tracer,times(1)).span(anyString(), any()); + } @Test diff --git a/src/test/java/com/spotify/github/v3/clients/IssueClientTest.java b/src/test/java/com/spotify/github/v3/clients/IssueClientTest.java index 3d3cc719..313ffdc8 100644 --- a/src/test/java/com/spotify/github/v3/clients/IssueClientTest.java +++ b/src/test/java/com/spotify/github/v3/clients/IssueClientTest.java @@ -31,9 +31,10 @@ import static java.util.stream.StreamSupport.stream; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.core.Is.is; +import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; -import static org.mockito.Mockito.when; +import static org.mockito.Mockito.*; import static org.powermock.api.mockito.PowerMockito.mock; import com.google.common.collect.Lists; @@ -58,13 +59,14 @@ public class IssueClientTest { private GitHubClient github; private IssueClient issueClient; + private NoopTracer tracer = mock(NoopTracer.class); @Before public void setUp() { github = mock(GitHubClient.class); when(github.json()).thenReturn(Json.create()); when(github.urlFor("")).thenReturn("https://github.com/api/v3"); - issueClient = new IssueClient(github, "someowner", "somerepo"); + issueClient = new IssueClient(github, "someowner", "somerepo").withTracer(tracer); } @Test @@ -141,5 +143,7 @@ public void testCommentCreated() throws IOException { final Comment comment = issueClient.createComment(10, "Me too").join(); assertThat(comment.id(), is(114)); + verify(tracer,times(1)).span(anyString(), any()); + } } diff --git a/src/test/java/com/spotify/github/v3/clients/PullRequestClientTest.java b/src/test/java/com/spotify/github/v3/clients/PullRequestClientTest.java index 1d79e765..9166ff40 100644 --- a/src/test/java/com/spotify/github/v3/clients/PullRequestClientTest.java +++ b/src/test/java/com/spotify/github/v3/clients/PullRequestClientTest.java @@ -24,9 +24,7 @@ import static java.nio.charset.Charset.defaultCharset; import static org.junit.Assert.assertEquals; import static org.mockito.ArgumentMatchers.any; -import static org.mockito.Mockito.doNothing; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; +import static org.mockito.Mockito.*; import com.google.common.collect.ImmutableList; import com.google.common.io.Resources; @@ -53,6 +51,7 @@ public class PullRequestClientTest { private GitHubClient github; private OkHttpClient client; + private NoopTracer tracer = mock(NoopTracer.class); private static String getFixture(String resource) throws IOException { return Resources.toString(getResource(PullRequestClientTest.class, resource), defaultCharset()); @@ -149,7 +148,7 @@ public void testRemoveRequestedReview_failure() throws Throwable { when(client.newCall(any())).thenReturn(call); PullRequestClient pullRequestClient = - PullRequestClient.create(github, "owner", "repo"); + PullRequestClient.create(github, "owner", "repo").withTracer(tracer); CompletableFuture result = pullRequestClient.removeRequestedReview(1, ImmutableRequestReviewParameters.builder() @@ -157,6 +156,7 @@ public void testRemoveRequestedReview_failure() throws Throwable { .build()); capture.getValue().onResponse(call, response); + verify(tracer,times(1)).span(any(), any()); try { result.get(); diff --git a/src/test/java/com/spotify/github/v3/clients/RepositoryClientTest.java b/src/test/java/com/spotify/github/v3/clients/RepositoryClientTest.java index 5f214551..4191d60e 100644 --- a/src/test/java/com/spotify/github/v3/clients/RepositoryClientTest.java +++ b/src/test/java/com/spotify/github/v3/clients/RepositoryClientTest.java @@ -37,13 +37,15 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import static org.mockito.ArgumentMatchers.any; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; import static java.util.stream.StreamSupport.stream; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.*; +import static org.mockito.Mockito.times; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Lists; import com.google.common.io.Resources; +import com.spotify.github.Tracer; import com.spotify.github.async.AsyncPage; import com.spotify.github.jackson.Json; import com.spotify.github.v3.comment.Comment; @@ -63,6 +65,8 @@ import java.util.Optional; import java.util.concurrent.CompletableFuture; import java.util.stream.Collectors; + +import io.opencensus.trace.Tracing; import okhttp3.Headers; import okhttp3.MediaType; import okhttp3.Protocol; @@ -82,6 +86,7 @@ public class RepositoryClientTest { private GitHubClient github; private RepositoryClient repoClient; private Json json; + private NoopTracer noopTracer = mock(NoopTracer.class); private static String getFixture(String resource) throws IOException { return Resources.toString(getResource(RepositoryTest.class, resource), defaultCharset()); @@ -90,7 +95,7 @@ private static String getFixture(String resource) throws IOException { @Before public void setUp() { github = mock(GitHubClient.class); - repoClient = new RepositoryClient(github, "someowner", "somerepo"); + repoClient = new RepositoryClient(github, "someowner", "somerepo").withTracer(noopTracer); json = Json.create(); when(github.json()).thenReturn(json); } @@ -107,6 +112,8 @@ public void getRepository() throws Exception { assertThat(repository.fullName(), is(repository.owner().login() + "/Hello-World")); assertThat(repository.isPrivate(), is(false)); assertThat(repository.fork(), is(false)); + verify(noopTracer,times(1)).span(eq("Get repository"), any()); + } @Test diff --git a/src/test/java/com/spotify/github/v3/clients/SearchClientTest.java b/src/test/java/com/spotify/github/v3/clients/SearchClientTest.java index 53d74d10..5594dcf7 100644 --- a/src/test/java/com/spotify/github/v3/clients/SearchClientTest.java +++ b/src/test/java/com/spotify/github/v3/clients/SearchClientTest.java @@ -25,8 +25,7 @@ import static com.spotify.github.v3.search.SearchTest.assertSearchIssues; import static java.nio.charset.Charset.defaultCharset; import static java.util.concurrent.CompletableFuture.completedFuture; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; +import static org.mockito.Mockito.*; import com.google.common.io.Resources; import com.spotify.github.jackson.Json; @@ -44,6 +43,8 @@ public class SearchClientTest { private SearchClient searchClient; private Json json; + private NoopTracer tracer = mock(NoopTracer.class); + private static String getFixture(String resource) throws IOException { return Resources.toString(getResource(SearchTest.class, resource), defaultCharset()); } @@ -51,7 +52,7 @@ private static String getFixture(String resource) throws IOException { @Before public void setUp() { github = mock(GitHubClient.class); - searchClient = SearchClient.create(github); + searchClient = SearchClient.create(github).withTracer(tracer); json = Json.create(); } @@ -63,6 +64,8 @@ public void testSearchIssue() throws Exception { when(github.request(ISSUES_URI + "?q=bogus-q", SearchIssues.class)).thenReturn(fixture); final SearchIssues search = searchClient.issues(ImmutableSearchParameters.builder().q("bogus-q").build()).get(); + + verify(tracer,times(1)).span(eq("Search issues"), any()); assertSearchIssues(search); } } From ae9ceff48d2d350792c86fabdecb2ecedfba24e0 Mon Sep 17 00:00:00 2001 From: Dennis Granath Date: Mon, 18 Oct 2021 16:38:03 +0200 Subject: [PATCH 5/7] Put tracing inside github call instead --- src/main/java/com/spotify/github/Tracer.java | 2 +- .../github/opencensus/OpenCensusTracer.java | 14 ++- .../github/v3/clients/ChecksClient.java | 32 +---- .../github/v3/clients/GitDataClient.java | 51 ++------ .../github/v3/clients/GitHubClient.java | 10 +- .../github/v3/clients/GithubAppClient.java | 23 +--- .../github/v3/clients/IssueClient.java | 23 +--- .../spotify/github/v3/clients/NoopTracer.java | 3 +- .../github/v3/clients/PullRequestClient.java | 51 ++------ .../github/v3/clients/RepositoryClient.java | 117 +++++------------- .../github/v3/clients/SearchClient.java | 18 +-- .../github/v3/clients/ChecksClientTest.java | 10 +- .../github/v3/clients/GitDataClientTest.java | 11 +- .../github/v3/clients/GitHubClientTest.java | 9 +- .../github/v3/clients/IssueClientTest.java | 8 +- .../v3/clients/PullRequestClientTest.java | 8 +- .../v3/clients/RepositoryClientTest.java | 13 +- .../github/v3/clients/SearchClientTest.java | 9 +- 18 files changed, 112 insertions(+), 300 deletions(-) diff --git a/src/main/java/com/spotify/github/Tracer.java b/src/main/java/com/spotify/github/Tracer.java index d7a9c3e2..5d5bcd9c 100644 --- a/src/main/java/com/spotify/github/Tracer.java +++ b/src/main/java/com/spotify/github/Tracer.java @@ -26,7 +26,7 @@ public interface Tracer { /** Create scoped span. Span will be closed when future completes. */ Span span( - String name, CompletionStage future); + String path, String method, CompletionStage future); } diff --git a/src/main/java/com/spotify/github/opencensus/OpenCensusTracer.java b/src/main/java/com/spotify/github/opencensus/OpenCensusTracer.java index 3a451d78..9ff8f1d3 100644 --- a/src/main/java/com/spotify/github/opencensus/OpenCensusTracer.java +++ b/src/main/java/com/spotify/github/opencensus/OpenCensusTracer.java @@ -35,27 +35,29 @@ public class OpenCensusTracer implements Tracer { private static final io.opencensus.trace.Tracer TRACER = Tracing.getTracer(); @Override - public Span span(final String name, final CompletionStage future) { - return internalSpan(name, future); + public Span span(final String name, final String method, final CompletionStage future) { + return internalSpan(name, method, future); } @SuppressWarnings("MustBeClosedChecker") private Span internalSpan( - final String name, + final String path, + final String method, final CompletionStage future) { - requireNonNull(name); + requireNonNull(path); requireNonNull(future); final io.opencensus.trace.Span ocSpan = - TRACER.spanBuilder(name).setSpanKind(CLIENT).startSpan(); + TRACER.spanBuilder("GitHub Request").setSpanKind(CLIENT).startSpan(); ocSpan.putAttribute("component", stringAttributeValue("github-api-client")); ocSpan.putAttribute("peer.service", stringAttributeValue("github")); + ocSpan.putAttribute("path", stringAttributeValue(path)); + ocSpan.putAttribute("method", stringAttributeValue(method)); final Span span = new OpenCensusSpan(ocSpan); future.whenComplete( (result, t) -> { - if (t == null) { span.success(); } else { diff --git a/src/main/java/com/spotify/github/v3/clients/ChecksClient.java b/src/main/java/com/spotify/github/v3/clients/ChecksClient.java index c1a1417c..70439199 100644 --- a/src/main/java/com/spotify/github/v3/clients/ChecksClient.java +++ b/src/main/java/com/spotify/github/v3/clients/ChecksClient.java @@ -21,7 +21,6 @@ package com.spotify.github.v3.clients; import com.google.common.collect.ImmutableMap; -import com.spotify.github.Tracer; import com.spotify.github.v3.checks.CheckRunRequest; import com.spotify.github.v3.checks.CheckRunResponse; import com.spotify.github.v3.checks.CheckRunResponseList; @@ -48,8 +47,6 @@ public class ChecksClient { private final Map extraHeaders = ImmutableMap.of(HttpHeaders.ACCEPT, "application/vnd.github.antiope-preview+json"); - private Tracer tracer = NoopTracer.INSTANCE; - /** * Instantiates a new Checks client. * @@ -63,11 +60,6 @@ public class ChecksClient { this.repo = repo; } - public ChecksClient withTracer(final Tracer tracer) { - this.tracer = tracer; - return this; - } - /** * Create a checkRun. * @@ -76,10 +68,8 @@ public ChecksClient withTracer(final Tracer tracer) { */ public CompletableFuture createCheckRun(final CheckRunRequest checkRun) { final String path = String.format(CHECK_RUNS_URI, owner, repo); - CompletableFuture future = github.post( + return github.post( path, github.json().toJsonUnchecked(checkRun), CheckRunResponse.class, extraHeaders); - tracer.span("Create checkrun", future); - return future; } /** @@ -92,10 +82,8 @@ public CompletableFuture createCheckRun(final CheckRunRequest public CompletableFuture updateCheckRun( final int id, final CheckRunRequest checkRun) { final String path = String.format(GET_CHECK_RUN_URI, owner, repo, id); - CompletableFuture future = github.patch( + return github.patch( path, github.json().toJsonUnchecked(checkRun), CheckRunResponse.class, extraHeaders); - tracer.span("Update checkrun", future); - return future; } /** @@ -106,9 +94,7 @@ public CompletableFuture updateCheckRun( */ public CompletableFuture getCheckRun(final int id) { final String path = String.format(GET_CHECK_RUN_URI, owner, repo, id); - CompletableFuture future = github.request(path, CheckRunResponse.class, extraHeaders); - tracer.span("Get checkrun", future); - return future; + return github.request(path, CheckRunResponse.class, extraHeaders); } /** @@ -119,9 +105,7 @@ public CompletableFuture getCheckRun(final int id) { */ public CompletableFuture getCheckRuns(final String ref) { final String path = String.format(LIST_CHECK_RUNS_URI, owner, repo, ref); - CompletableFuture future = github.request(path, CheckRunResponseList.class, extraHeaders); - tracer.span("List checkruns", future); - return future; + return github.request(path, CheckRunResponseList.class, extraHeaders); } /** @@ -132,9 +116,7 @@ public CompletableFuture getCheckRuns(final String ref) { */ public CompletableFuture getCheckSuite(final String id) { final String path = String.format(GET_CHECK_SUITE_URI, owner, repo, id); - CompletableFuture future = github.request(path, CheckSuite.class, extraHeaders); - tracer.span("Get check suite", future); - return future; + return github.request(path, CheckSuite.class, extraHeaders); } /** @@ -145,8 +127,6 @@ public CompletableFuture getCheckSuite(final String id) { */ public CompletableFuture getCheckSuites(final String sha) { final String path = String.format(LIST_CHECK_SUITES_REF_URI, owner, repo, sha); - CompletableFuture future = github.request(path, CheckSuiteResponseList.class, extraHeaders); - tracer.span("List check suites", future); - return future; + return github.request(path, CheckSuiteResponseList.class, extraHeaders); } } diff --git a/src/main/java/com/spotify/github/v3/clients/GitDataClient.java b/src/main/java/com/spotify/github/v3/clients/GitDataClient.java index 97587ef4..760a474c 100644 --- a/src/main/java/com/spotify/github/v3/clients/GitDataClient.java +++ b/src/main/java/com/spotify/github/v3/clients/GitDataClient.java @@ -26,7 +26,6 @@ import static java.lang.String.format; import com.google.common.collect.ImmutableMap; -import com.spotify.github.Tracer; import com.spotify.github.v3.git.Reference; import com.spotify.github.v3.git.ShaLink; import com.spotify.github.v3.git.Tag; @@ -58,7 +57,6 @@ public class GitDataClient { private final GitHubClient github; private final String owner; private final String repo; - private Tracer tracer = NoopTracer.INSTANCE; GitDataClient(final GitHubClient github, final String owner, final String repo) { this.github = github; @@ -70,11 +68,6 @@ static GitDataClient create(final GitHubClient github, final String owner, final return new GitDataClient(github, owner, repo); } - public GitDataClient withTracer(final Tracer tracer) { - this.tracer = tracer; - return this; - } - /** * Deletes a git reference. * @@ -82,9 +75,7 @@ public GitDataClient withTracer(final Tracer tracer) { */ public CompletableFuture deleteReference(final String ref) { final String path = format(REFERENCE_URI, owner, repo, ref.replaceAll("refs/", "")); - CompletableFuture future = github.delete(path).thenAccept(IGNORE_RESPONSE_CONSUMER); - tracer.span("Delete reference", future); - return future; + return github.delete(path).thenAccept(IGNORE_RESPONSE_CONSUMER); } /** @@ -112,9 +103,7 @@ public CompletableFuture deleteTag(final String tag) { */ public CompletableFuture getBranchReference(final String branch) { final String path = format(BRANCH_REFERENCE_URI, owner, repo, branch); - CompletableFuture future = github.request(path, Reference.class); - tracer.span("Get branch reference", future); - return future; + return github.request(path, Reference.class); } /** @@ -124,9 +113,7 @@ public CompletableFuture getBranchReference(final String branch) { */ public CompletableFuture getTagReference(final String tag) { final String path = format(TAG_REFERENCE_URI, owner, repo, tag); - CompletableFuture future = github.request(path, Reference.class); - tracer.span("Get tag reference", future); - return future; + return github.request(path, Reference.class); } /** @@ -136,9 +123,7 @@ public CompletableFuture getTagReference(final String tag) { */ public CompletableFuture getTag(final String tag) { final String path = format(TAG_URI, owner, repo, tag); - CompletableFuture future = github.request(path, Tag.class); - tracer.span("Get annotated tag", future); - return future; + return github.request(path, Tag.class); } /** @@ -148,9 +133,7 @@ public CompletableFuture getTag(final String tag) { */ public CompletableFuture> listMatchingReferences(final String ref) { final String path = format(LIST_MATCHING_REFERENCES_URI, owner, repo, ref); - CompletableFuture> future = github.request(path, LIST_REFERENCES); - tracer.span("List matching references", future); - return future; + return github.request(path, LIST_REFERENCES); } /** @@ -182,9 +165,7 @@ public CompletableFuture createReference(final String ref, final Stri of( "ref", ref, "sha", sha); - CompletableFuture future = github.post(path, github.json().toJsonUnchecked(body), Reference.class); - tracer.span("Create git reference", future); - return future; + return github.post(path, github.json().toJsonUnchecked(body), Reference.class); } /** @@ -254,9 +235,7 @@ public CompletableFuture createCommit( .json() .toJsonUnchecked( ImmutableMap.of("message", message, "parents", parents, "tree", treeSha)); - CompletableFuture future = github.post(path, requestBody, Commit.class); - tracer.span("Create commit", future); - return future; + return github.post(path, requestBody, Commit.class); } /** @@ -267,9 +246,7 @@ public CompletableFuture createCommit( */ public CompletableFuture getTree(final String sha) { final String path = String.format(TREE_SHA_URI_TEMPLATE, owner, repo, sha); - CompletableFuture future = github.request(path, Tree.class); - tracer.span("Get repository tree", future); - return future; + return github.request(path, Tree.class); } /** @@ -280,9 +257,7 @@ public CompletableFuture getTree(final String sha) { */ public CompletableFuture getRecursiveTree(final String sha) { final String path = String.format(TREE_SHA_URI_TEMPLATE, owner, repo, sha); - CompletableFuture future = github.request(path + "?recursive=true", Tree.class); - tracer.span("Get recursive tree", future); - return future; + return github.request(path + "?recursive=true", Tree.class); } /** @@ -296,9 +271,7 @@ public CompletableFuture createTree(final List tree, final Strin final String path = String.format(TREE_URI_TEMPLATE, owner, repo); final String requestBody = github.json() .toJsonUnchecked(ImmutableMap.of("base_tree", baseTreeSha, "tree", tree)); - CompletableFuture future = github.post(path, requestBody, Tree.class); - tracer.span("Create tree", future); - return future; + return github.post(path, requestBody, Tree.class); } @@ -312,9 +285,7 @@ public CompletableFuture createBlob(final String content) { final String encoding = "utf-8|base64"; final String requestBody = github.json() .toJsonUnchecked(ImmutableMap.of("content", content, "encoding", encoding)); - CompletableFuture future = github.post(path, requestBody, ShaLink.class); - tracer.span("Create blob", future); - return future; + return github.post(path, requestBody, ShaLink.class); } diff --git a/src/main/java/com/spotify/github/v3/clients/GitHubClient.java b/src/main/java/com/spotify/github/v3/clients/GitHubClient.java index 23d6699f..40170820 100644 --- a/src/main/java/com/spotify/github/v3/clients/GitHubClient.java +++ b/src/main/java/com/spotify/github/v3/clients/GitHubClient.java @@ -24,6 +24,7 @@ import static okhttp3.MediaType.parse; import com.fasterxml.jackson.core.type.TypeReference; +import com.spotify.github.Tracer; import com.spotify.github.jackson.Json; import com.spotify.github.v3.checks.AccessToken; import com.spotify.github.v3.comment.Comment; @@ -65,6 +66,8 @@ */ public class GitHubClient { + private Tracer tracer = NoopTracer.INSTANCE; + static final Consumer IGNORE_RESPONSE_CONSUMER = (response) -> { if (response.body() != null) { response.body().close(); @@ -300,6 +303,11 @@ static String responseBodyUnchecked(final Response response) { } } + public GitHubClient withTracer(final Tracer tracer) { + this.tracer = tracer; + return this; + } + public Optional getPrivateKey() { return Optional.ofNullable(privateKey); } @@ -724,7 +732,7 @@ public void onResponse(final Call call, final Response response) { }); } }); - + tracer.span(request.url().toString(), request.method(), future); return future; } diff --git a/src/main/java/com/spotify/github/v3/clients/GithubAppClient.java b/src/main/java/com/spotify/github/v3/clients/GithubAppClient.java index b639e025..bcc075a5 100644 --- a/src/main/java/com/spotify/github/v3/clients/GithubAppClient.java +++ b/src/main/java/com/spotify/github/v3/clients/GithubAppClient.java @@ -22,7 +22,6 @@ import com.fasterxml.jackson.core.type.TypeReference; import com.google.common.collect.ImmutableMap; -import com.spotify.github.Tracer; import com.spotify.github.v3.apps.InstallationRepositoriesResponse; import com.spotify.github.v3.checks.AccessToken; import com.spotify.github.v3.checks.Installation; @@ -42,7 +41,6 @@ public class GithubAppClient { private final GitHubClient github; private final String owner; private final String repo; - private Tracer tracer = NoopTracer.INSTANCE; private final Map extraHeaders = ImmutableMap.of(HttpHeaders.ACCEPT, "application/vnd.github.machine-man-preview+json"); @@ -56,20 +54,13 @@ public class GithubAppClient { this.repo = repo; } - public GithubAppClient withTracer(final Tracer tracer) { - this.tracer = tracer; - return this; - } - /** * List Installations of an app. * * @return a list of Installation */ public CompletableFuture> getInstallations() { - CompletableFuture> future = github.request(GET_INSTALLATIONS_URL, INSTALLATION_LIST_TYPE_REFERENCE, extraHeaders); - tracer.span("List Installations", future); - return future; + return github.request(GET_INSTALLATIONS_URL, INSTALLATION_LIST_TYPE_REFERENCE, extraHeaders); } /** @@ -78,10 +69,8 @@ public CompletableFuture> getInstallations() { * @return a list of Installation */ public CompletableFuture getInstallation() { - CompletableFuture future = github.request( + return github.request( String.format(GET_INSTALLATION_REPO_URL, owner, repo), Installation.class, extraHeaders); - tracer.span("Get installation", future); - return future; } /** @@ -91,9 +80,7 @@ public CompletableFuture getInstallation() { */ public CompletableFuture getAccessToken(final Integer installationId) { final String path = String.format(GET_ACCESS_TOKEN_URL, installationId); - CompletableFuture future = github.post(path, "", AccessToken.class, extraHeaders); - tracer.span("Get access token", future); - return future; + return github.post(path, "", AccessToken.class, extraHeaders); } /** @@ -105,9 +92,7 @@ public CompletableFuture getAccessToken(final Integer installationI public CompletableFuture listAccessibleRepositories( final int installationId) { - CompletableFuture future = GitHubClient.scopeForInstallationId(github, installationId) + return GitHubClient.scopeForInstallationId(github, installationId) .request(LIST_ACCESSIBLE_REPOS_URL, InstallationRepositoriesResponse.class, extraHeaders); - tracer.span("List scoped repositories", future); - return future; } } diff --git a/src/main/java/com/spotify/github/v3/clients/IssueClient.java b/src/main/java/com/spotify/github/v3/clients/IssueClient.java index 2352e32e..1d965d27 100644 --- a/src/main/java/com/spotify/github/v3/clients/IssueClient.java +++ b/src/main/java/com/spotify/github/v3/clients/IssueClient.java @@ -24,7 +24,6 @@ import static com.spotify.github.v3.clients.GitHubClient.LIST_COMMENT_TYPE_REFERENCE; import com.google.common.collect.ImmutableMap; -import com.spotify.github.Tracer; import com.spotify.github.async.AsyncPage; import com.spotify.github.v3.comment.Comment; import java.lang.invoke.MethodHandles; @@ -44,7 +43,6 @@ public class IssueClient { private final GitHubClient github; private final String owner; private final String repo; - private Tracer tracer = NoopTracer.INSTANCE; IssueClient(final GitHubClient github, final String owner, final String repo) { this.github = github; @@ -56,11 +54,6 @@ static IssueClient create(final GitHubClient github, final String owner, final S return new IssueClient(github, owner, repo); } - public IssueClient withTracer(final Tracer tracer) { - this.tracer = tracer; - return this; - } - /** * List repository comments. * @@ -89,9 +82,7 @@ public Iterator> listComments(final int number) { public CompletableFuture getComment(final int id) { final String path = String.format(COMMENTS_URI_ID_TEMPLATE, owner, repo, id); log.info("Fetching issue comments from " + path); - CompletableFuture future = github.request(path, Comment.class); - tracer.span("Get comment", future); - return future; + return github.request(path, Comment.class); } /** @@ -104,9 +95,7 @@ public CompletableFuture getComment(final int id) { public CompletableFuture createComment(final int number, final String body) { final String path = String.format(COMMENTS_URI_NUMBER_TEMPLATE, owner, repo, number); final String requestBody = github.json().toJsonUnchecked(ImmutableMap.of("body", body)); - CompletableFuture future = github.post(path, requestBody, Comment.class); - tracer.span("Create comment", future); - return future; + return github.post(path, requestBody, Comment.class); } /** @@ -117,11 +106,9 @@ public CompletableFuture createComment(final int number, final String b */ public CompletableFuture editComment(final int id, final String body) { final String path = String.format(COMMENTS_URI_ID_TEMPLATE, owner, repo, id); - CompletableFuture future = github + return github .patch(path, github.json().toJsonUnchecked(ImmutableMap.of("body", body))) .thenAccept(IGNORE_RESPONSE_CONSUMER); - tracer.span("Edit comment", future); - return future; } /** @@ -130,11 +117,9 @@ public CompletableFuture editComment(final int id, final String body) { * @param id comment id */ public CompletableFuture deleteComment(final int id) { - CompletableFuture future = github + return github .delete(String.format(COMMENTS_URI_ID_TEMPLATE, owner, repo, id)) .thenAccept(IGNORE_RESPONSE_CONSUMER); - tracer.span("Delete comment", future); - return future; } private Iterator> listComments(final String path) { diff --git a/src/main/java/com/spotify/github/v3/clients/NoopTracer.java b/src/main/java/com/spotify/github/v3/clients/NoopTracer.java index 16493933..961d1de3 100644 --- a/src/main/java/com/spotify/github/v3/clients/NoopTracer.java +++ b/src/main/java/com/spotify/github/v3/clients/NoopTracer.java @@ -47,7 +47,8 @@ private NoopTracer() {} @Override public Span span( - final String name, + final String path, + final String method, final CompletionStage future) { return SPAN; } diff --git a/src/main/java/com/spotify/github/v3/clients/PullRequestClient.java b/src/main/java/com/spotify/github/v3/clients/PullRequestClient.java index 75d2536d..00ed9974 100644 --- a/src/main/java/com/spotify/github/v3/clients/PullRequestClient.java +++ b/src/main/java/com/spotify/github/v3/clients/PullRequestClient.java @@ -27,7 +27,6 @@ import static com.spotify.github.v3.clients.GitHubClient.LIST_REVIEW_TYPE_REFERENCE; import com.google.common.base.Strings; -import com.spotify.github.Tracer; import com.spotify.github.async.AsyncPage; import com.spotify.github.v3.prs.*; import com.spotify.github.v3.prs.requests.PullRequestCreate; @@ -54,7 +53,6 @@ public class PullRequestClient { private final GitHubClient github; private final String owner; private final String repo; - private Tracer tracer = NoopTracer.INSTANCE; PullRequestClient(final GitHubClient github, final String owner, final String repo) { this.github = github; @@ -67,11 +65,6 @@ static PullRequestClient create( return new PullRequestClient(github, owner, repo); } - public PullRequestClient withTracer(final Tracer tracer) { - this.tracer = tracer; - return this; - } - /** * List repository pull request. * @@ -102,9 +95,7 @@ public CompletableFuture> list(final PullRequestParameters public CompletableFuture get(final int number) { final String path = String.format(PR_NUMBER_TEMPLATE, owner, repo, number); log.debug("Fetching pull request from " + path); - CompletableFuture future = github.request(path, PullRequest.class); - tracer.span("Get a specific pull request", future); - return future; + return github.request(path, PullRequest.class); } /** @@ -114,11 +105,9 @@ public CompletableFuture get(final int number) { */ public CompletableFuture create(final PullRequestCreate request) { final String path = String.format(PR_TEMPLATE, owner, repo); - CompletableFuture future = github + return github .post(path, github.json().toJsonUnchecked(request)) .thenAccept(IGNORE_RESPONSE_CONSUMER); - tracer.span("Create a pull request", future); - return future; } /** @@ -129,11 +118,9 @@ public CompletableFuture create(final PullRequestCreate request) { */ public CompletableFuture update(final int number, final PullRequestUpdate request) { final String path = String.format(PR_NUMBER_TEMPLATE, owner, repo, number); - CompletableFuture future = github + return github .patch(path, github.json().toJsonUnchecked(request)) .thenAccept(IGNORE_RESPONSE_CONSUMER); - tracer.span("Update given pull request", future); - return future; } /** @@ -145,9 +132,7 @@ public CompletableFuture update(final int number, final PullRequestUpdate public CompletableFuture> listCommits(final int number) { final String path = String.format(PR_COMMITS_TEMPLATE, owner, repo, number); log.debug("Fetching pull request commits from " + path); - CompletableFuture> future = github.request(path, LIST_COMMIT_TYPE_REFERENCE); - tracer.span("List pull request commits", future); - return future; + return github.request(path, LIST_COMMIT_TYPE_REFERENCE); } /** @@ -159,9 +144,7 @@ public CompletableFuture> listCommits(final int number) { public CompletableFuture> listReviews(final int number) { final String path = String.format(PR_REVIEWS_TEMPLATE, owner, repo, number); log.debug("Fetching pull request reviews from " + path); - CompletableFuture> future = github.request(path, LIST_REVIEW_TYPE_REFERENCE); - tracer.span("List pull request reviews", future); - return future; + return github.request(path, LIST_REVIEW_TYPE_REFERENCE); } /** @@ -189,9 +172,7 @@ public CompletableFuture createReview(final int number, final ReviewPara final String path = String.format(PR_REVIEWS_TEMPLATE, owner, repo, number); final String jsonPayload = github.json().toJsonUnchecked(properties); log.debug("Creating review for PR: " + path); - CompletableFuture future = github.post(path, jsonPayload, Review.class); - tracer.span("Create a review for a pull request", future); - return future; + return github.post(path, jsonPayload, Review.class); } /** @@ -203,9 +184,7 @@ public CompletableFuture createReview(final int number, final ReviewPara public CompletableFuture listReviewRequests(final int number) { final String path = String.format(PR_REVIEW_REQUESTS_TEMPLATE, owner, repo, number); log.debug("Fetching pull request requested reviews from " + path); - CompletableFuture future = github.request(path, LIST_REVIEW_REQUEST_TYPE_REFERENCE); - tracer.span("List pull request requested reviews", future); - return future; + return github.request(path, LIST_REVIEW_REQUEST_TYPE_REFERENCE); } /** @@ -219,9 +198,7 @@ public CompletableFuture requestReview(final int number, final Requ final String path = String.format(PR_REVIEW_REQUESTS_TEMPLATE, owner, repo, number); final String jsonPayload = github.json().toJsonUnchecked(properties); log.debug("Requesting reviews for PR: " + path); - CompletableFuture future = github.post(path, jsonPayload, PullRequest.class); - tracer.span("Request review for a pull request", future); - return future; + return github.post(path, jsonPayload, PullRequest.class); } /** @@ -235,9 +212,7 @@ public CompletableFuture removeRequestedReview(final int number, final Req final String path = String.format(PR_REVIEW_REQUESTS_TEMPLATE, owner, repo, number); final String jsonPayload = github.json().toJsonUnchecked(properties); log.debug("Removing requested reviews for PR: " + path); - CompletableFuture future = github.delete(path, jsonPayload).thenAccept(IGNORE_RESPONSE_CONSUMER); - tracer.span("Remove request for review for a pull request", future); - return future; + return github.delete(path, jsonPayload).thenAccept(IGNORE_RESPONSE_CONSUMER); } /** @@ -251,17 +226,13 @@ public CompletableFuture merge(final int number, final MergeParameters pro final String path = String.format(PR_NUMBER_TEMPLATE + "/merge", owner, repo, number); final String jsonPayload = github.json().toJsonUnchecked(properties); log.debug("Merging pr, running: {}", path); - CompletableFuture future = github.put(path, jsonPayload).thenAccept(IGNORE_RESPONSE_CONSUMER); - tracer.span("Merge pull request", future); - return future; + return github.put(path, jsonPayload).thenAccept(IGNORE_RESPONSE_CONSUMER); } private CompletableFuture> list(final String parameterPath) { final String path = String.format(PR_TEMPLATE + parameterPath, owner, repo); log.debug("Fetching pull requests from " + path); - CompletableFuture> future = github.request(path, LIST_PR_TYPE_REFERENCE); - tracer.span("List pull requests", future); - return future; + return github.request(path, LIST_PR_TYPE_REFERENCE); } } diff --git a/src/main/java/com/spotify/github/v3/clients/RepositoryClient.java b/src/main/java/com/spotify/github/v3/clients/RepositoryClient.java index a5c3a287..fd806256 100644 --- a/src/main/java/com/spotify/github/v3/clients/RepositoryClient.java +++ b/src/main/java/com/spotify/github/v3/clients/RepositoryClient.java @@ -26,11 +26,9 @@ import static com.spotify.github.v3.clients.GitHubClient.LIST_FOLDERCONTENT_TYPE_REFERENCE; import static com.spotify.github.v3.clients.GitHubClient.LIST_REPOSITORY; import static com.spotify.github.v3.clients.GitHubClient.LIST_STATUS_TYPE_REFERENCE; -import static java.util.Objects.requireNonNull; import com.google.common.base.Strings; import com.google.common.collect.ImmutableMap; -import com.spotify.github.Tracer; import com.spotify.github.async.AsyncPage; import com.spotify.github.v3.comment.Comment; import com.spotify.github.v3.exceptions.RequestNotOkException; @@ -87,7 +85,6 @@ public class RepositoryClient { private final String owner; private final String repo; private final GitHubClient github; - private Tracer tracer = NoopTracer.INSTANCE; RepositoryClient(final GitHubClient github, final String owner, final String repo) { this.github = github; @@ -99,18 +96,13 @@ static RepositoryClient create(final GitHubClient github, final String owner, fi return new RepositoryClient(github, owner, repo); } - public RepositoryClient withTracer(final Tracer tracer) { - this.tracer = requireNonNull(tracer); - return this; - } - /** * Create an issue API client. * * @return issue API client */ public IssueClient createIssueClient() { - return IssueClient.create(github, owner, repo).withTracer(tracer); + return IssueClient.create(github, owner, repo); } /** @@ -119,7 +111,7 @@ public IssueClient createIssueClient() { * @return pull request API client */ public PullRequestClient createPullRequestClient() { - return PullRequestClient.create(github, owner, repo).withTracer(tracer); + return PullRequestClient.create(github, owner, repo); } /** @@ -128,7 +120,7 @@ public PullRequestClient createPullRequestClient() { * @return Github App API client */ public GithubAppClient createGithubAppClient() { - return new GithubAppClient(github, owner, repo).withTracer(tracer); + return new GithubAppClient(github, owner, repo); } /** @@ -140,7 +132,7 @@ public ChecksClient createChecksApiClient() { if (!github.getPrivateKey().isPresent()) { throw new IllegalArgumentException("Checks Client needs a private key"); } - return new ChecksClient(github, owner, repo).withTracer(tracer); + return new ChecksClient(github, owner, repo); } /** @@ -150,9 +142,7 @@ public ChecksClient createChecksApiClient() { */ public CompletableFuture getRepository() { final String path = String.format(REPOSITORY_URI_TEMPLATE, owner, repo); - CompletableFuture future = github.request(path, Repository.class); - tracer.span("Get repository", future); - return future; + return github.request(path, Repository.class); } /** @@ -162,9 +152,7 @@ public CompletableFuture getRepository() { */ public CompletableFuture> listOrganizationRepositories() { final String path = String.format(LIST_REPOSITORY_TEMPLATE, owner); - CompletableFuture> future = github.request(path, LIST_REPOSITORY); - tracer.span("List repositories", future); - return future; + return github.request(path, LIST_REPOSITORY); } /** @@ -190,9 +178,7 @@ public Iterator> listAuthenticatedUserRepositories( */ public CompletableFuture isCollaborator(final String user) { final String path = String.format(IS_USER_COLLABORATOR_OF_REPO, owner, repo, user); - CompletableFuture future = github.request(path).thenApply(response -> response.code() == NO_CONTENT); - tracer.span("isCollaborator", future); - return future; + return github.request(path).thenApply(response -> response.code() == NO_CONTENT); } /** @@ -204,7 +190,8 @@ public CompletableFuture isCollaborator(final String user) { public CompletableFuture createWebhook( final WebhookCreate request, final boolean ignoreExisting) { final String path = String.format(HOOK_URI_TEMPLATE, owner, repo); - CompletableFuture future = github + + return github .post(path, github.json().toJsonUnchecked(request)) .thenAccept(IGNORE_RESPONSE_CONSUMER) .exceptionally( @@ -224,8 +211,6 @@ public CompletableFuture createWebhook( throw new CompletionException(e); }); - tracer.span("Create webhook", future); - return future; } /** @@ -237,11 +222,9 @@ public CompletableFuture createWebhook( public CompletableFuture setCommitStatus( final String sha, final RepositoryCreateStatus request) { final String path = String.format(STATUS_URI_TEMPLATE, owner, repo, sha); - CompletableFuture future = github + return github .post(path, github.json().toJsonUnchecked(request)) .thenAccept(IGNORE_RESPONSE_CONSUMER); - tracer.span("Set commit status", future); - return future; } /** @@ -251,9 +234,7 @@ public CompletableFuture setCommitStatus( */ public CompletableFuture getCommitStatus(final String ref) { final String path = String.format(COMMIT_STATUS_URI_TEMPLATE, owner, repo, ref); - CompletableFuture future = github.request(path, CommitStatus.class); - tracer.span("Get commit status", future); - return future; + return github.request(path, CommitStatus.class); } /** @@ -264,9 +245,7 @@ public CompletableFuture getCommitStatus(final String ref) { */ public CompletableFuture> listCommitStatuses(final String sha) { final String path = String.format(STATUS_URI_TEMPLATE, owner, repo, sha); - CompletableFuture> future = github.request(path, LIST_STATUS_TYPE_REFERENCE); - tracer.span("List commit statuses", future); - return future; + return github.request(path, LIST_STATUS_TYPE_REFERENCE); } /** @@ -291,9 +270,7 @@ public Iterator> listCommitStatuses(final String sha, final in */ public CompletableFuture> listCommits() { final String path = String.format(COMMITS_URI_TEMPLATE, owner, repo); - CompletableFuture> future = github.request(path, LIST_COMMIT_TYPE_REFERENCE); - tracer.span("List commits", future); - return future; + return github.request(path, LIST_COMMIT_TYPE_REFERENCE); } /** @@ -304,9 +281,7 @@ public CompletableFuture> listCommits() { */ public CompletableFuture getCommit(final String sha) { final String path = String.format(COMMIT_SHA_URI_TEMPLATE, owner, repo, sha); - CompletableFuture future = github.request(path, Commit.class); - tracer.span("Get commit", future); - return future; + return github.request(path, Commit.class); } /** @@ -319,9 +294,7 @@ public CompletableFuture getCommit(final String sha) { @Deprecated public CompletableFuture getTree(final String sha) { final String path = String.format(TREE_SHA_URI_TEMPLATE, owner, repo, sha); - CompletableFuture future = github.request(path, Tree.class); - tracer.span("Get tree", future); - return future; + return github.request(path, Tree.class); } /** @@ -331,9 +304,7 @@ public CompletableFuture getTree(final String sha) { * @return content */ public CompletableFuture getFileContent(final String path) { - CompletableFuture future = github.request(getContentPath(path, ""), Content.class); - tracer.span("Get file content", future); - return future; + return github.request(getContentPath(path, ""), Content.class); } /** @@ -344,9 +315,7 @@ public CompletableFuture getFileContent(final String path) { * @return content */ public CompletableFuture getFileContent(final String path, final String ref) { - CompletableFuture future = github.request(getContentPath(path, "?ref=" + ref), Content.class); - tracer.span("Get file content", future); - return future; + return github.request(getContentPath(path, "?ref=" + ref), Content.class); } /** @@ -356,9 +325,7 @@ public CompletableFuture getFileContent(final String path, final String * @return content */ public CompletableFuture> getFolderContent(final String path) { - CompletableFuture> future = github.request(getContentPath(path, ""), LIST_FOLDERCONTENT_TYPE_REFERENCE); - tracer.span("Get folder content", future); - return future; + return github.request(getContentPath(path, ""), LIST_FOLDERCONTENT_TYPE_REFERENCE); } /** @@ -371,9 +338,7 @@ public CompletableFuture> getFolderContent(final String path public CompletableFuture createComment(final String sha, final String body) { final String path = String.format(CREATE_COMMENT_TEMPLATE, owner, repo, sha); final String requestBody = github.json().toJsonUnchecked(ImmutableMap.of("body", body)); - CompletableFuture future = github.post(path, requestBody, Comment.class); - tracer.span("Create comment", future); - return future; + return github.post(path, requestBody, Comment.class); } /** @@ -384,9 +349,7 @@ public CompletableFuture createComment(final String sha, final String b */ public CompletableFuture getComment(final int id) { final String path = String.format(COMMENT_TEMPLATE, owner, repo, id); - CompletableFuture future = github.request(path, Comment.class); - tracer.span("Get comment", future); - return future; + return github.request(path, Comment.class); } /** @@ -398,9 +361,7 @@ public CompletableFuture getComment(final int id) { */ public CompletableFuture> getFolderContent( final String path, final String ref) { - CompletableFuture> future = github.request(getContentPath(path, "?ref=" + ref), LIST_FOLDERCONTENT_TYPE_REFERENCE); - tracer.span("Get folder content", future); - return future; + return github.request(getContentPath(path, "?ref=" + ref), LIST_FOLDERCONTENT_TYPE_REFERENCE); } /** @@ -412,9 +373,7 @@ public CompletableFuture> getFolderContent( */ public CompletableFuture compareCommits(final String base, final String head) { final String path = String.format(COMPARE_COMMIT_TEMPLATE, owner, repo, base, head); - CompletableFuture future = github.request(path, CommitComparison.class); - tracer.span("Compare commits", future); - return future; + return github.request(path, CommitComparison.class); } /** @@ -425,9 +384,7 @@ public CompletableFuture compareCommits(final String base, fin */ public CompletableFuture getBranch(final String branch) { final String path = String.format(BRANCH_TEMPLATE, owner, repo, branch); - CompletableFuture future = github.request(path, Branch.class); - tracer.span("Get branch", future); - return future; + return github.request(path, Branch.class); } /** @@ -437,9 +394,7 @@ public CompletableFuture getBranch(final String branch) { */ public CompletableFuture> listBranches() { final String path = String.format(LIST_BRANCHES_TEMPLATE, owner, repo); - CompletableFuture> future = github.request(path, LIST_BRANCHES); - tracer.span("List branches", future); - return future; + return github.request(path, LIST_BRANCHES); } /** @@ -449,9 +404,7 @@ public CompletableFuture> listBranches() { */ public CompletableFuture deleteComment(final int id) { final String path = String.format(COMMENT_TEMPLATE, owner, repo, id); - CompletableFuture future = github.delete(path).thenAccept(IGNORE_RESPONSE_CONSUMER); - tracer.span("Delete comment", future); - return future; + return github.delete(path).thenAccept(IGNORE_RESPONSE_CONSUMER); } /** @@ -462,11 +415,9 @@ public CompletableFuture deleteComment(final int id) { */ public CompletableFuture editComment(final int id, final String body) { final String path = String.format(COMMENT_TEMPLATE, owner, repo, id); - CompletableFuture future = github + return github .patch(path, github.json().toJsonUnchecked(ImmutableMap.of("body", body))) .thenAccept(IGNORE_RESPONSE_CONSUMER); - tracer.span("Edit comment", future); - return future; } /** @@ -476,9 +427,7 @@ public CompletableFuture editComment(final int id, final String body) { */ public CompletableFuture getLanguages() { final String path = String.format(LANGUAGES_TEMPLATE, owner, repo); - CompletableFuture future = github.request(path, Languages.class); - tracer.span("Get languages", future); - return future; + return github.request(path, Languages.class); } /** @@ -490,9 +439,7 @@ public CompletableFuture getLanguages() { * @return resulting merge commit, or empty if base already contains the head (nothing to merge) */ public CompletableFuture> merge(final String base, final String head) { - CompletableFuture> future = merge(base, head, null); - tracer.span("Merge", future); - return future; + return merge(base, head, null); } /** @@ -513,7 +460,7 @@ public CompletableFuture> merge( : ImmutableMap.of("base", base, "head", head, "commit_message", commitMessage); final String body = github.json().toJsonUnchecked(params); - CompletableFuture> future = github + return github .post(path, body) .thenApply( response -> { @@ -531,8 +478,6 @@ public CompletableFuture> merge( GitHubClient.responseBodyUnchecked(response), CommitItem.class); return Optional.of(commitItem); }); - tracer.span("Merge", future); - return future; } /** @@ -548,7 +493,7 @@ public CompletableFuture createFork(final String organization) { (organization == null) ? ImmutableMap.of() : ImmutableMap.of("organization", organization); final String body = github.json().toJsonUnchecked(params); - CompletableFuture future = github + return github .post(path, body) .thenApply( response -> { @@ -559,8 +504,6 @@ public CompletableFuture createFork(final String organization) { GitHubClient.responseBodyUnchecked(response), Repository.class); return repositoryItem; }); - tracer.span("Create fork", future); - return future; } private String getContentPath(final String path, final String query) { diff --git a/src/main/java/com/spotify/github/v3/clients/SearchClient.java b/src/main/java/com/spotify/github/v3/clients/SearchClient.java index 7f2fb59f..764a9657 100644 --- a/src/main/java/com/spotify/github/v3/clients/SearchClient.java +++ b/src/main/java/com/spotify/github/v3/clients/SearchClient.java @@ -21,7 +21,6 @@ package com.spotify.github.v3.clients; import com.google.common.base.Strings; -import com.spotify.github.Tracer; import com.spotify.github.v3.search.SearchIssues; import com.spotify.github.v3.search.SearchRepositories; import com.spotify.github.v3.search.SearchUsers; @@ -39,7 +38,6 @@ public class SearchClient { static final String REPOSITORIES_URI = "/search/repositories"; static final String ISSUES_URI = "/search/issues"; private final GitHubClient github; - private Tracer tracer = NoopTracer.INSTANCE; SearchClient(final GitHubClient github) { this.github = github; @@ -49,10 +47,6 @@ static SearchClient create(final GitHubClient github) { return new SearchClient(github); } - public SearchClient withTracer(final Tracer tracer) { - this.tracer = tracer; - return this; - } /** * Search users. * @@ -60,9 +54,7 @@ public SearchClient withTracer(final Tracer tracer) { * @return user search results */ public CompletableFuture users(final SearchParameters parameters) { - CompletableFuture future = search(USERS_URI, parameters, SearchUsers.class); - tracer.span("Search users", future); - return future; + return search(USERS_URI, parameters, SearchUsers.class); } /** @@ -72,9 +64,7 @@ public CompletableFuture users(final SearchParameters parameters) { * @return issue search results */ public CompletableFuture issues(final SearchParameters parameters) { - CompletableFuture future = search(ISSUES_URI, parameters, SearchIssues.class); - tracer.span("Search issues", future); - return future; + return search(ISSUES_URI, parameters, SearchIssues.class); } /** @@ -84,9 +74,7 @@ public CompletableFuture issues(final SearchParameters parameters) * @return repository search results */ public CompletableFuture repositories(final SearchParameters parameters) { - CompletableFuture future = search(REPOSITORIES_URI, parameters, SearchRepositories.class); - tracer.span("Search repositories", future); - return future; + return search(REPOSITORIES_URI, parameters, SearchRepositories.class); } private CompletableFuture search( diff --git a/src/test/java/com/spotify/github/v3/clients/ChecksClientTest.java b/src/test/java/com/spotify/github/v3/clients/ChecksClientTest.java index 93824872..46cb2517 100644 --- a/src/test/java/com/spotify/github/v3/clients/ChecksClientTest.java +++ b/src/test/java/com/spotify/github/v3/clients/ChecksClientTest.java @@ -28,14 +28,12 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.core.Is.is; import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; -import static org.mockito.Mockito.*; -import static org.mockito.Mockito.times; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; import com.google.common.io.Resources; import com.spotify.github.jackson.Json; -import com.spotify.github.opencensus.OpenCensusTracer; import com.spotify.github.v3.checks.CheckRunOutput; import com.spotify.github.v3.checks.CheckRunRequest; import com.spotify.github.v3.checks.CheckRunResponse; @@ -54,7 +52,6 @@ public class ChecksClientTest { private GitHubClient github; private ChecksClient checksClient; private Json json; - private OpenCensusTracer tracer = mock(OpenCensusTracer.class); public static String loadResource(final String path) { try { @@ -67,7 +64,7 @@ public static String loadResource(final String path) { @Before public void setUp() { github = mock(GitHubClient.class); - checksClient = new ChecksClient(github, "someowner", "somerepo").withTracer(tracer); + checksClient = new ChecksClient(github, "someowner", "somerepo"); json = Json.create(); when(github.json()).thenReturn(json); } @@ -93,7 +90,6 @@ public void createCompletedCheckRun() throws Exception { assertThat(actualResponse.get().status(), is(completed)); assertThat(actualResponse.get().headSha(), is("ce587453ced02b1526dfb4cb910479d431683101")); assertThat(actualResponse.get().output().annotationsCount().get(), is(2)); - verify(tracer,times(1)).span(anyString(), any()); } @Test diff --git a/src/test/java/com/spotify/github/v3/clients/GitDataClientTest.java b/src/test/java/com/spotify/github/v3/clients/GitDataClientTest.java index 0f3fbf94..3874dd74 100644 --- a/src/test/java/com/spotify/github/v3/clients/GitDataClientTest.java +++ b/src/test/java/com/spotify/github/v3/clients/GitDataClientTest.java @@ -31,10 +31,9 @@ import static org.hamcrest.core.Is.is; import static org.hamcrest.core.StringContains.containsString; import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; -import static org.mockito.Mockito.*; -import static org.mockito.Mockito.times; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; import com.google.common.collect.ImmutableMap; import com.google.common.io.Resources; @@ -59,7 +58,6 @@ public class GitDataClientTest { private GitHubClient github; private GitDataClient gitDataClient; private Json json; - private NoopTracer tracer = mock(NoopTracer.class); private static String getFixture(String resource) throws IOException { return Resources.toString(getResource(GitDataClientTest.class, resource), defaultCharset()); @@ -68,7 +66,7 @@ private static String getFixture(String resource) throws IOException { @Before public void setUp() { github = mock(GitHubClient.class); - gitDataClient = new GitDataClient(github, "someowner", "somerepo").withTracer(tracer); + gitDataClient = new GitDataClient(github, "someowner", "somerepo"); json = Json.create(); when(github.json()).thenReturn(json); } @@ -81,7 +79,6 @@ public void getTagRef() throws Exception { .thenReturn(fixture); final Reference reference = gitDataClient.getTagReference("0.0.1").get(); assertThat(reference.object().sha(), is("5926dd300de5fee31d445c57be223f00e128a634")); - verify(tracer,times(1)).span(anyString(), any()); } @Test @@ -273,8 +270,6 @@ public void testGetRecursiveTree() throws IOException { .join(); assertThat(tree.sha(), is("9c27bd92524e2b57b569d4c86695b3993d9b8f9f")); assertThat(tree.tree().size(), is(7)); - verify(tracer,times(1)).span(anyString(), any()); - } @Test diff --git a/src/test/java/com/spotify/github/v3/clients/GitHubClientTest.java b/src/test/java/com/spotify/github/v3/clients/GitHubClientTest.java index 182a7a96..8e4504d1 100644 --- a/src/test/java/com/spotify/github/v3/clients/GitHubClientTest.java +++ b/src/test/java/com/spotify/github/v3/clients/GitHubClientTest.java @@ -25,10 +25,9 @@ import static org.hamcrest.core.Is.is; import static org.junit.Assert.fail; import static org.mockito.ArgumentMatchers.any; -import static org.mockito.Mockito.doNothing; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; +import static org.mockito.Mockito.*; +import com.spotify.github.Tracer; import com.spotify.github.v3.exceptions.ReadOnlyRepositoryException; import com.spotify.github.v3.exceptions.RequestNotOkException; import com.spotify.github.v3.repos.CommitItem; @@ -52,6 +51,7 @@ public class GitHubClientTest { private GitHubClient github; private OkHttpClient client; + private Tracer tracer = mock(Tracer.class); @Before public void setUp() { @@ -88,10 +88,11 @@ public void testSearchIssue() throws Throwable { when(client.newCall(any())).thenReturn(call); IssueClient issueClient = - github.createRepositoryClient("testorg", "testrepo").createIssueClient(); + github.withTracer(tracer).createRepositoryClient("testorg", "testrepo").createIssueClient(); CompletableFuture maybeSucceeded = issueClient.editComment(1, "some comment"); capture.getValue().onResponse(call, response); + verify(tracer,times(1)).span(anyString(), anyString(),any()); try { maybeSucceeded.get(); } catch (Exception e) { diff --git a/src/test/java/com/spotify/github/v3/clients/IssueClientTest.java b/src/test/java/com/spotify/github/v3/clients/IssueClientTest.java index 313ffdc8..3d3cc719 100644 --- a/src/test/java/com/spotify/github/v3/clients/IssueClientTest.java +++ b/src/test/java/com/spotify/github/v3/clients/IssueClientTest.java @@ -31,10 +31,9 @@ import static java.util.stream.StreamSupport.stream; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.core.Is.is; -import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; -import static org.mockito.Mockito.*; +import static org.mockito.Mockito.when; import static org.powermock.api.mockito.PowerMockito.mock; import com.google.common.collect.Lists; @@ -59,14 +58,13 @@ public class IssueClientTest { private GitHubClient github; private IssueClient issueClient; - private NoopTracer tracer = mock(NoopTracer.class); @Before public void setUp() { github = mock(GitHubClient.class); when(github.json()).thenReturn(Json.create()); when(github.urlFor("")).thenReturn("https://github.com/api/v3"); - issueClient = new IssueClient(github, "someowner", "somerepo").withTracer(tracer); + issueClient = new IssueClient(github, "someowner", "somerepo"); } @Test @@ -143,7 +141,5 @@ public void testCommentCreated() throws IOException { final Comment comment = issueClient.createComment(10, "Me too").join(); assertThat(comment.id(), is(114)); - verify(tracer,times(1)).span(anyString(), any()); - } } diff --git a/src/test/java/com/spotify/github/v3/clients/PullRequestClientTest.java b/src/test/java/com/spotify/github/v3/clients/PullRequestClientTest.java index 9166ff40..1d79e765 100644 --- a/src/test/java/com/spotify/github/v3/clients/PullRequestClientTest.java +++ b/src/test/java/com/spotify/github/v3/clients/PullRequestClientTest.java @@ -24,7 +24,9 @@ import static java.nio.charset.Charset.defaultCharset; import static org.junit.Assert.assertEquals; import static org.mockito.ArgumentMatchers.any; -import static org.mockito.Mockito.*; +import static org.mockito.Mockito.doNothing; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; import com.google.common.collect.ImmutableList; import com.google.common.io.Resources; @@ -51,7 +53,6 @@ public class PullRequestClientTest { private GitHubClient github; private OkHttpClient client; - private NoopTracer tracer = mock(NoopTracer.class); private static String getFixture(String resource) throws IOException { return Resources.toString(getResource(PullRequestClientTest.class, resource), defaultCharset()); @@ -148,7 +149,7 @@ public void testRemoveRequestedReview_failure() throws Throwable { when(client.newCall(any())).thenReturn(call); PullRequestClient pullRequestClient = - PullRequestClient.create(github, "owner", "repo").withTracer(tracer); + PullRequestClient.create(github, "owner", "repo"); CompletableFuture result = pullRequestClient.removeRequestedReview(1, ImmutableRequestReviewParameters.builder() @@ -156,7 +157,6 @@ public void testRemoveRequestedReview_failure() throws Throwable { .build()); capture.getValue().onResponse(call, response); - verify(tracer,times(1)).span(any(), any()); try { result.get(); diff --git a/src/test/java/com/spotify/github/v3/clients/RepositoryClientTest.java b/src/test/java/com/spotify/github/v3/clients/RepositoryClientTest.java index 4191d60e..5f214551 100644 --- a/src/test/java/com/spotify/github/v3/clients/RepositoryClientTest.java +++ b/src/test/java/com/spotify/github/v3/clients/RepositoryClientTest.java @@ -37,15 +37,13 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; import static java.util.stream.StreamSupport.stream; -import static org.mockito.ArgumentMatchers.eq; -import static org.mockito.Mockito.*; -import static org.mockito.Mockito.times; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Lists; import com.google.common.io.Resources; -import com.spotify.github.Tracer; import com.spotify.github.async.AsyncPage; import com.spotify.github.jackson.Json; import com.spotify.github.v3.comment.Comment; @@ -65,8 +63,6 @@ import java.util.Optional; import java.util.concurrent.CompletableFuture; import java.util.stream.Collectors; - -import io.opencensus.trace.Tracing; import okhttp3.Headers; import okhttp3.MediaType; import okhttp3.Protocol; @@ -86,7 +82,6 @@ public class RepositoryClientTest { private GitHubClient github; private RepositoryClient repoClient; private Json json; - private NoopTracer noopTracer = mock(NoopTracer.class); private static String getFixture(String resource) throws IOException { return Resources.toString(getResource(RepositoryTest.class, resource), defaultCharset()); @@ -95,7 +90,7 @@ private static String getFixture(String resource) throws IOException { @Before public void setUp() { github = mock(GitHubClient.class); - repoClient = new RepositoryClient(github, "someowner", "somerepo").withTracer(noopTracer); + repoClient = new RepositoryClient(github, "someowner", "somerepo"); json = Json.create(); when(github.json()).thenReturn(json); } @@ -112,8 +107,6 @@ public void getRepository() throws Exception { assertThat(repository.fullName(), is(repository.owner().login() + "/Hello-World")); assertThat(repository.isPrivate(), is(false)); assertThat(repository.fork(), is(false)); - verify(noopTracer,times(1)).span(eq("Get repository"), any()); - } @Test diff --git a/src/test/java/com/spotify/github/v3/clients/SearchClientTest.java b/src/test/java/com/spotify/github/v3/clients/SearchClientTest.java index 5594dcf7..53d74d10 100644 --- a/src/test/java/com/spotify/github/v3/clients/SearchClientTest.java +++ b/src/test/java/com/spotify/github/v3/clients/SearchClientTest.java @@ -25,7 +25,8 @@ import static com.spotify.github.v3.search.SearchTest.assertSearchIssues; import static java.nio.charset.Charset.defaultCharset; import static java.util.concurrent.CompletableFuture.completedFuture; -import static org.mockito.Mockito.*; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; import com.google.common.io.Resources; import com.spotify.github.jackson.Json; @@ -43,8 +44,6 @@ public class SearchClientTest { private SearchClient searchClient; private Json json; - private NoopTracer tracer = mock(NoopTracer.class); - private static String getFixture(String resource) throws IOException { return Resources.toString(getResource(SearchTest.class, resource), defaultCharset()); } @@ -52,7 +51,7 @@ private static String getFixture(String resource) throws IOException { @Before public void setUp() { github = mock(GitHubClient.class); - searchClient = SearchClient.create(github).withTracer(tracer); + searchClient = SearchClient.create(github); json = Json.create(); } @@ -64,8 +63,6 @@ public void testSearchIssue() throws Exception { when(github.request(ISSUES_URI + "?q=bogus-q", SearchIssues.class)).thenReturn(fixture); final SearchIssues search = searchClient.issues(ImmutableSearchParameters.builder().q("bogus-q").build()).get(); - - verify(tracer,times(1)).span(eq("Search issues"), any()); assertSearchIssues(search); } } From 661bd15eaa585a7077276251887e41a880711169 Mon Sep 17 00:00:00 2001 From: Dennis Granath Date: Tue, 19 Oct 2021 10:24:09 +0200 Subject: [PATCH 6/7] Opencensus tracer test --- pom.xml | 14 +- .../opencensus/OpenCensusTracerTest.java | 132 ++++++++++++++++++ .../github/opencensus/TestExportHandler.java | 82 +++++++++++ 3 files changed, 227 insertions(+), 1 deletion(-) create mode 100644 src/test/java/com/spotify/github/opencensus/OpenCensusTracerTest.java create mode 100644 src/test/java/com/spotify/github/opencensus/TestExportHandler.java diff --git a/pom.xml b/pom.xml index dfc7e59b..12def3f9 100644 --- a/pom.xml +++ b/pom.xml @@ -100,7 +100,7 @@ 2.8.3 2.0.2 3.0.1 - 0.22.1 + 0.26.0 ${project.groupId}.githubclient.shade @@ -224,6 +224,18 @@ ${mockito-core.version} test + + io.opencensus + opencensus-testing + ${opencensus.version} + test + + + io.opencensus + opencensus-impl + ${opencensus.version} + test + com.squareup.okhttp3 mockwebserver diff --git a/src/test/java/com/spotify/github/opencensus/OpenCensusTracerTest.java b/src/test/java/com/spotify/github/opencensus/OpenCensusTracerTest.java new file mode 100644 index 00000000..d09602fd --- /dev/null +++ b/src/test/java/com/spotify/github/opencensus/OpenCensusTracerTest.java @@ -0,0 +1,132 @@ +/*- + * -\-\- + * github-client + * -- + * Copyright (C) 2016 - 2021 Spotify AB + * -- + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * -/-/- + */ + +package com.spotify.github.opencensus; + + +import io.grpc.Context; +import io.opencensus.trace.*; +import io.opencensus.trace.config.TraceConfig; +import io.opencensus.trace.config.TraceParams; +import io.opencensus.trace.export.SpanData; +import io.opencensus.trace.samplers.Samplers; +import io.opencensus.trace.unsafe.ContextUtils; +import org.junit.Before; +import org.junit.BeforeClass; +import org.junit.Test; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.BeforeEach; + +import java.util.List; +import java.util.Map; +import java.util.concurrent.CompletableFuture; + +import static io.opencensus.trace.AttributeValue.stringAttributeValue; +import static org.junit.jupiter.api.Assertions.assertEquals; + +public class OpenCensusTracerTest { + + + private final String rootSpanName = "root span"; + private TestExportHandler spanExporterHandler; + + /** + * Test that trace() a) returns a future that completes when the input future completes and b) + * sets up the Spans appropriately so that the Span for the operation is exported with the + * rootSpan set as the parent. + */ + @Test + public void testTrace_CompletionStage_Simple() throws Exception { + Span rootSpan = startRootSpan(); + final CompletableFuture future = new CompletableFuture<>(); + OpenCensusTracer tracer = new OpenCensusTracer(); + + tracer.span("path", "GET", future); + future.complete("all done"); + rootSpan.end(); + + List exportedSpans = spanExporterHandler.waitForSpansToBeExported(2); + assertEquals(2, exportedSpans.size()); + + SpanData root = findSpan(exportedSpans, rootSpanName); + SpanData inner = findSpan(exportedSpans, "GitHub Request"); + + assertEquals(root.getContext().getTraceId(), inner.getContext().getTraceId()); + assertEquals(root.getContext().getSpanId(), inner.getParentSpanId()); + final Map attributes = inner.getAttributes().getAttributeMap(); + assertEquals(stringAttributeValue("github-api-client"), attributes.get("component")); + assertEquals(stringAttributeValue("github"), attributes.get("peer.service")); + assertEquals(stringAttributeValue("path"), attributes.get("path")); + assertEquals(stringAttributeValue("GET"), attributes.get("method")); + assertEquals(Status.OK, inner.getStatus()); + } + + @Test + public void testTrace_CompletionStage_Fails() throws Exception { + Span rootSpan = startRootSpan(); + final CompletableFuture future = new CompletableFuture<>(); + OpenCensusTracer tracer = new OpenCensusTracer(); + + tracer.span("path", "POST", future); + future.completeExceptionally(new Exception("GitHub failed!")); + rootSpan.end(); + + List exportedSpans = spanExporterHandler.waitForSpansToBeExported(2); + assertEquals(2, exportedSpans.size()); + + SpanData root = findSpan(exportedSpans, rootSpanName); + SpanData inner = findSpan(exportedSpans, "GitHub Request"); + + assertEquals(root.getContext().getTraceId(), inner.getContext().getTraceId()); + assertEquals(root.getContext().getSpanId(), inner.getParentSpanId()); + final Map attributes = inner.getAttributes().getAttributeMap(); + assertEquals(stringAttributeValue("github-api-client"), attributes.get("component")); + assertEquals(stringAttributeValue("github"), attributes.get("peer.service")); + assertEquals(stringAttributeValue("path"), attributes.get("path")); + assertEquals(stringAttributeValue("POST"), attributes.get("method")); + assertEquals(Status.UNKNOWN, inner.getStatus()); + } + + private Span startRootSpan() { + Span rootSpan = Tracing.getTracer().spanBuilder(rootSpanName).startSpan(); + Context context = ContextUtils.withValue(Context.current(), rootSpan); + context.attach(); + return rootSpan; + } + + private SpanData findSpan(final List spans, final String name) { + return spans.stream().filter(s -> s.getName().equals(name)).findFirst().get(); + } + + @Before + public void setUpExporter() { + spanExporterHandler = new TestExportHandler(); + Tracing.getExportComponent().getSpanExporter().registerHandler("test", spanExporterHandler); + } + + @BeforeClass + public static void setupTracing() { + final TraceConfig traceConfig = Tracing.getTraceConfig(); + final Sampler sampler = Samplers.alwaysSample(); + final TraceParams newParams = + traceConfig.getActiveTraceParams().toBuilder().setSampler(sampler).build(); + traceConfig.updateActiveTraceParams(newParams); + } +} \ No newline at end of file diff --git a/src/test/java/com/spotify/github/opencensus/TestExportHandler.java b/src/test/java/com/spotify/github/opencensus/TestExportHandler.java new file mode 100644 index 00000000..c1519572 --- /dev/null +++ b/src/test/java/com/spotify/github/opencensus/TestExportHandler.java @@ -0,0 +1,82 @@ +/*- + * -\-\- + * github-client + * -- + * Copyright (C) 2016 - 2021 Spotify AB + * -- + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * -/-/- + */ + +package com.spotify.github.opencensus; + +import io.opencensus.trace.export.SpanData; +import io.opencensus.trace.export.SpanExporter; +import java.time.Duration; +import java.time.Instant; +import java.util.ArrayList; +import java.util.Collection; +import java.util.List; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * A dummy SpanExporter.Handler which keeps any exported Spans in memory, so we can query against + * them in tests. + * + *

The opencensus-testing library has a TestHandler that can be used in tests like this, but the + * only method it exposes to gain access to the received spans is waitForExport(int) which blocks + * forever until the given number of spans is exported, which could be never. So instead we define + * our own very simple implementation. + */ +class TestExportHandler extends SpanExporter.Handler { + private static final Logger LOG = LoggerFactory.getLogger(TestExportHandler.class); + + private final List receivedSpans = new ArrayList<>(); + private final Object lock = new Object(); + + @Override + public void export(final Collection spanDataList) { + synchronized (lock) { + receivedSpans.addAll(spanDataList); + LOG.info("received {} spans, {} total", spanDataList.size(), receivedSpans.size()); + } + } + + List receivedSpans() { + synchronized (lock) { + return new ArrayList<>(receivedSpans); + } + } + + /** Wait up to waitTime for at least `count` spans to be exported */ + List waitForSpansToBeExported(final int count) throws InterruptedException { + // opencensus is hardcoded to export batches every 5 seconds (see + // io.opencensus.implcore.trace.export.ExportComponentImpl), so wait slightly longer than that + Duration waitTime = Duration.ofSeconds(7); + Instant deadline = Instant.now().plus(waitTime); + + List spanData = receivedSpans(); + while (spanData.size() < count) { + //noinspection BusyWait + Thread.sleep(100); + spanData = receivedSpans(); + + if (!Instant.now().isBefore(deadline)) { + LOG.warn("ending busy wait for spans because deadline passed"); + break; + } + } + return spanData; + } +} From 4c6403c8f4ec845ef7a3911fa4240f951536c586 Mon Sep 17 00:00:00 2001 From: Dennis Granath Date: Tue, 19 Oct 2021 11:47:56 +0200 Subject: [PATCH 7/7] Set status_code and message on request exceptions --- src/main/java/com/spotify/github/Span.java | 2 +- .../github/opencensus/OpenCensusSpan.java | 14 +++++++++++++- .../github/opencensus/OpenCensusTracer.java | 4 ++-- .../spotify/github/v3/clients/NoopTracer.java | 2 +- .../github/opencensus/OpenCensusSpanTest.java | 19 ++++++++++++++++++- .../opencensus/OpenCensusTracerTest.java | 4 ++-- 6 files changed, 37 insertions(+), 8 deletions(-) diff --git a/src/main/java/com/spotify/github/Span.java b/src/main/java/com/spotify/github/Span.java index f8a24aeb..70b2e939 100644 --- a/src/main/java/com/spotify/github/Span.java +++ b/src/main/java/com/spotify/github/Span.java @@ -24,7 +24,7 @@ public interface Span extends AutoCloseable { Span success(); - Span failure(); + Span failure(Throwable t); /** Close span. Must be called for any opened span. */ @Override diff --git a/src/main/java/com/spotify/github/opencensus/OpenCensusSpan.java b/src/main/java/com/spotify/github/opencensus/OpenCensusSpan.java index 9b8a4872..a48fcb09 100644 --- a/src/main/java/com/spotify/github/opencensus/OpenCensusSpan.java +++ b/src/main/java/com/spotify/github/opencensus/OpenCensusSpan.java @@ -21,10 +21,14 @@ package com.spotify.github.opencensus; import static java.util.Objects.requireNonNull; import com.spotify.github.Span; +import com.spotify.github.v3.exceptions.RequestNotOkException; +import io.opencensus.trace.AttributeValue; import io.opencensus.trace.Status; class OpenCensusSpan implements Span { + public static final int NOT_FOUND = 404; + public static final int INTERNAL_SERVER_ERROR = 500; private final io.opencensus.trace.Span span; OpenCensusSpan(final io.opencensus.trace.Span span) { @@ -38,7 +42,15 @@ public Span success() { } @Override - public Span failure() { + public Span failure(final Throwable t) { + if (t instanceof RequestNotOkException) { + RequestNotOkException ex = (RequestNotOkException) t; + span.putAttribute("http.status_code", AttributeValue.longAttributeValue(ex.statusCode())); + span.putAttribute("message", AttributeValue.stringAttributeValue(ex.getMessage())); + if (ex.statusCode() - INTERNAL_SERVER_ERROR >= 0) { + span.putAttribute("error", AttributeValue.booleanAttributeValue(true)); + } + } span.setStatus(Status.UNKNOWN); return this; } diff --git a/src/main/java/com/spotify/github/opencensus/OpenCensusTracer.java b/src/main/java/com/spotify/github/opencensus/OpenCensusTracer.java index 9ff8f1d3..d4834b34 100644 --- a/src/main/java/com/spotify/github/opencensus/OpenCensusTracer.java +++ b/src/main/java/com/spotify/github/opencensus/OpenCensusTracer.java @@ -52,7 +52,7 @@ private Span internalSpan( ocSpan.putAttribute("component", stringAttributeValue("github-api-client")); ocSpan.putAttribute("peer.service", stringAttributeValue("github")); - ocSpan.putAttribute("path", stringAttributeValue(path)); + ocSpan.putAttribute("http.url", stringAttributeValue(path)); ocSpan.putAttribute("method", stringAttributeValue(method)); final Span span = new OpenCensusSpan(ocSpan); @@ -61,7 +61,7 @@ private Span internalSpan( if (t == null) { span.success(); } else { - span.failure(); + span.failure(t); } span.close(); }); diff --git a/src/main/java/com/spotify/github/v3/clients/NoopTracer.java b/src/main/java/com/spotify/github/v3/clients/NoopTracer.java index 961d1de3..5b3c769e 100644 --- a/src/main/java/com/spotify/github/v3/clients/NoopTracer.java +++ b/src/main/java/com/spotify/github/v3/clients/NoopTracer.java @@ -35,7 +35,7 @@ public Span success() { } @Override - public Span failure() { + public Span failure(final Throwable t) { return this; } diff --git a/src/test/java/com/spotify/github/opencensus/OpenCensusSpanTest.java b/src/test/java/com/spotify/github/opencensus/OpenCensusSpanTest.java index c3fb3b30..59428b7e 100644 --- a/src/test/java/com/spotify/github/opencensus/OpenCensusSpanTest.java +++ b/src/test/java/com/spotify/github/opencensus/OpenCensusSpanTest.java @@ -21,8 +21,12 @@ package com.spotify.github.opencensus; import com.spotify.github.Span; +import com.spotify.github.v3.exceptions.RequestNotOkException; +import io.opencensus.trace.AttributeValue; import io.opencensus.trace.Status; import org.junit.jupiter.api.Test; + +import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; @@ -42,10 +46,23 @@ public void succeed() { @Test public void fail() { final Span span = new OpenCensusSpan(wrapped); - span.failure(); + span.failure(new RequestNotOkException("path", 404, "Not found")); + span.close(); + + verify(wrapped).setStatus(Status.UNKNOWN); + verify(wrapped).putAttribute("http.status_code", AttributeValue.longAttributeValue(404)); + verify(wrapped).end(); + } + + @Test + public void failOnServerError() { + final Span span = new OpenCensusSpan(wrapped); + span.failure(new RequestNotOkException("path", 500, "Internal Server Error")); span.close(); verify(wrapped).setStatus(Status.UNKNOWN); + verify(wrapped).putAttribute("http.status_code", AttributeValue.longAttributeValue(500)); + verify(wrapped).putAttribute("error", AttributeValue.booleanAttributeValue(true)); verify(wrapped).end(); } diff --git a/src/test/java/com/spotify/github/opencensus/OpenCensusTracerTest.java b/src/test/java/com/spotify/github/opencensus/OpenCensusTracerTest.java index d09602fd..add341d6 100644 --- a/src/test/java/com/spotify/github/opencensus/OpenCensusTracerTest.java +++ b/src/test/java/com/spotify/github/opencensus/OpenCensusTracerTest.java @@ -73,7 +73,7 @@ public void testTrace_CompletionStage_Simple() throws Exception { final Map attributes = inner.getAttributes().getAttributeMap(); assertEquals(stringAttributeValue("github-api-client"), attributes.get("component")); assertEquals(stringAttributeValue("github"), attributes.get("peer.service")); - assertEquals(stringAttributeValue("path"), attributes.get("path")); + assertEquals(stringAttributeValue("path"), attributes.get("http.url")); assertEquals(stringAttributeValue("GET"), attributes.get("method")); assertEquals(Status.OK, inner.getStatus()); } @@ -99,7 +99,7 @@ public void testTrace_CompletionStage_Fails() throws Exception { final Map attributes = inner.getAttributes().getAttributeMap(); assertEquals(stringAttributeValue("github-api-client"), attributes.get("component")); assertEquals(stringAttributeValue("github"), attributes.get("peer.service")); - assertEquals(stringAttributeValue("path"), attributes.get("path")); + assertEquals(stringAttributeValue("path"), attributes.get("http.url")); assertEquals(stringAttributeValue("POST"), attributes.get("method")); assertEquals(Status.UNKNOWN, inner.getStatus()); }