Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[GP-4245] Debug logging for jira client #1290

Merged
merged 7 commits into from
Apr 23, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions changes/fix_jira-logging.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Additional server logging for jira errors

Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ public void labels(Set<String> labels) {
public ActionState perform(
ActionServices services, Duration lastGeneratedByOlive, boolean isOliveLive) {
if (connection == null) {
System.err.println("JIRA Connection for " + issueUrl + " is null.");
return ActionState.FAILED;
}
final var current = connection.get();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,13 @@ public ActionState perform(
.orElse(false)) {
bestMatch.accept(issue);
if (!connection.transition(issue, Stream::anyMatch, comment)) {
StringBuilder errorBuilder = new StringBuilder();
errorBuilder
.append("Unable to transition issue ")
.append(issue.getKey())
.append("\nConnection: ")
.append(connection);
System.err.println(errorBuilder);
return ActionState.FAILED;
}
}
Expand All @@ -68,6 +75,11 @@ public boolean search(Pattern query) {
public String verb() {
return "Close";
}

@Override
public String toString() {
return "Close JIRA Issue with comment " + comment;
}
}

public static class Open extends IssueVerb {
Expand Down Expand Up @@ -149,6 +161,19 @@ public boolean search(Pattern query) {
public String verb() {
return "Open";
}

@Override
public String toString() {
StringBuilder writeOut = new StringBuilder();
writeOut
.append("Re/open JIRA Issue with description '")
.append(description)
.append("', reopen with comment '")
.append(comment)
.append("'");
assignee.ifPresent(s -> writeOut.append(", with assignee '").append(s));
return writeOut.toString();
}
}

public abstract ActionState perform(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,16 @@ protected Stream<Issue> fetch(String jql, Instant lastUpdated)
throws URISyntaxException, IOException, InterruptedException {
return search(jql, FIELDS).stream();
}

@Override
protected String getKType() {
return "String";
}

@Override
protected String getVType() {
return "Stream<Issue>";
}
}

static class JiraActionFilter {
Expand Down Expand Up @@ -341,12 +351,12 @@ public <F> Stream<Pair<String, F>> searches(
.flatMap(
search ->
search
.getType()
.type()
.join(
search.getName(),
search.getFilter().convert(builder),
search.name(),
search.filter().convert(builder),
issues
.get(search.getJql())
.get(search.jql())
.flatMap(
issue -> {
final var assignee =
Expand Down Expand Up @@ -444,16 +454,26 @@ boolean transition(
authenticationHeader.ifPresent(
header -> commentRequestBuilder.header("Authorization", header));

return CLIENT
.send(
commentRequestBuilder
.header("Content-Type", "application/json")
.POST(BodyPublishers.ofString(MAPPER.writeValueAsString(updateComment)))
.build(),
BodyHandlers.discarding())
.statusCode()
/ 100
== 2;
var result =
CLIENT.send(
commentRequestBuilder
.header("Content-Type", "application/json")
.POST(BodyPublishers.ofString(MAPPER.writeValueAsString(updateComment)))
.build(),
BodyHandlers.ofString());
boolean isGood = result.statusCode() / 100 == 2;
if (!isGood) {
StringBuilder errorBuilder = new StringBuilder();
errorBuilder
.append("Unable to transition issue ")
.append(issue.getKey())
.append(" using any of ")
.append(transitions)
.append("\nGot ")
.append(result.body());
System.err.println(errorBuilder);
}
return isGood;
}
}
return false;
Expand Down Expand Up @@ -506,4 +526,34 @@ public Optional<Integer> update(Configuration config) {
public String url() {
return url;
}

@Override
public String toString() {
StringBuilder writeOut = new StringBuilder();
writeOut
.append("JiraConnection with closedStatuses = ")
.append(closedStatuses)
.append(", defaultFieldValues = ")
.append(defaultFieldValues.entrySet())
.append(", definer = ")
.append(definer)
.append(", issueTypeId = ")
.append(issueTypeId)
.append(", issueTypeName = ")
.append(issueTypeName)
.append(", cached issues = ")
.append(issues)
.append(", projectId = ")
.append(projectId)
.append(", projectKey = ")
.append(projectKey)
.append(", searches = ")
.append(searches)
.append(", url = ")
.append(url)
.append(", version = ")
.append(version);
authenticationHeader.ifPresent(s -> writeOut.append(", authenticationHeader = ").append(s));
return writeOut.toString();
}
}
39 changes: 1 addition & 38 deletions plugin-jira/src/main/java/ca/on/oicr/gsi/shesmu/jira/Search.java
Original file line number Diff line number Diff line change
Expand Up @@ -2,41 +2,4 @@

import ca.on.oicr.gsi.shesmu.plugin.filter.ActionFilter;

public final class Search {
private ActionFilter filter;
private String jql;
private String name;
private JoiningRule type;

public ActionFilter getFilter() {
return filter;
}

public String getJql() {
return jql;
}

public String getName() {
return name;
}

public JoiningRule getType() {
return type;
}

public void setFilter(ActionFilter filter) {
this.filter = filter;
}

public void setJql(String jql) {
this.jql = jql;
}

public void setName(String name) {
this.name = name;
}

public void setType(JoiningRule type) {
this.type = type;
}
}
public record Search(ActionFilter filter, String jql, String name, JoiningRule type) {}
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,16 @@ public synchronized Optional<Integer> update(Configuration configuration) {
protected Optional<Object> fetch(Tuple key, Instant lastUpdated) {
return Optional.of(function.apply(definer.get().connection.get(), key));
}

@Override
protected String getKType() {
return "Tuple";
}

@Override
protected String getVType() {
return "Optional<Object>";
}
};

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,16 @@ protected Optional<AlgebraicValue> fetch(Pair<Path, Boolean> fileName, Instant l
throw e;
}
}

@Override
protected String getKType() {
return "Pair<Path, Boolean>";
}

@Override
protected String getVType() {
return "Optional<AlgebraicValue>";
}
}

static final ObjectMapper MAPPER = new ObjectMapper();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,16 @@ protected Optional<Tuple> fetch(String id, Instant lastUpdated) throws Exception
}
return Optional.empty();
}

@Override
protected String getKType() {
return "String";
}

@Override
protected String getVType() {
return "Optional<Tuple>";
}
}

private static final Imyhat EXTERNAL_KEY_TYPE =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -169,4 +169,30 @@ public final void ttl(int ttl) {
this.ttl = ttl;
ttlValue.labels(name).set(ttl);
}

public String toString() {
StringBuilder writeOut = new StringBuilder();
writeOut
.append("KeyValueCache of types ")
.append(getKType())
.append(" and ")
.append(getVType())
.append(" with CACHES = ")
.append(CACHES.entrySet())
.append(", maxCount = ")
.append(maxCount)
.append(", name = ")
.append(name)
.append(", recordFactory = ")
.append(recordFactory)
.append(", records = ")
.append(records.entrySet())
.append(", ttl = ")
.append(ttl);
return writeOut.toString();
}

protected abstract String getKType();

protected abstract String getVType();
Copy link
Contributor

Choose a reason for hiding this comment

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

getKeyType and getValueType would be more clear, but you're also basically defining these at the class level. Would it be more useful to just log the class name in toString rather than the type parameters? That would also be more concise and impossible to mess up when implementing a new subclass

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I designed it this way to minimize repeat code, if I moved toString to the class level then there would be ~20 lines of repeat code in each child class of KeyValueCache

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant that "KType" and "VType" are being defined at the class level right now - the return value is a static String for all implementations. This means the class name is just as useful, if not more so since 2 classes could have the same types. In KeyValueCache#toString you could just log getClass() (or one of its String properties) instead of needing getKType and getVType methods

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't realize this would get the actual class name instead of the abstract class's name, thanks, fixed

}
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,9 @@ protected <F> F convert(
ActionFilterBuilder<F, ActionState, String, Instant, Long> filterBuilder) {
return filterBuilder.added(start, end);
}

@Override
protected String getName() {
return "Last generated by olive";
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,9 @@ protected <F> F convert(
long offset, ActionFilterBuilder<F, ActionState, String, Instant, Long> filterBuilder) {
return filterBuilder.addedAgo(offset);
}

@Override
protected String getOperation() {
return "Last generated by olive (ago)";
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,9 @@ public <F> F convert(
ActionFilterBuilder<F, ActionState, String, Instant, Long> filterBuilder, Stream<F> filters) {
return filterBuilder.and(filters);
}

@Override
protected String getOperation() {
return "AND";
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,9 @@ public <F> F convert(
long offset, ActionFilterBuilder<F, ActionState, String, Instant, Long> filterBuilder) {
return filterBuilder.checkedAgo(offset);
}

@Override
protected String getOperation() {
return "Last run by scheduler (ago)";
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,9 @@ public <F> F convert(
ActionFilterBuilder<F, ActionState, String, Instant, Long> filterBuilder) {
return filterBuilder.checked(start, end);
}

@Override
protected String getName() {
return "created";
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,9 @@ public <F> F convert(
long offset, ActionFilterBuilder<F, ActionState, String, Instant, Long> filterBuilder) {
return filterBuilder.createdAgo(offset);
}

@Override
protected String getOperation() {
return "created (ago)";
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,9 @@ public <F> F convert(
ActionFilterBuilder<F, ActionState, String, Instant, Long> filterBuilder) {
return filterBuilder.external(start, end);
}

@Override
protected String getName() {
return "External Modification";
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,9 @@ public <F> F convert(
long offset, ActionFilterBuilder<F, ActionState, String, Instant, Long> filterBuilder) {
return filterBuilder.externalAgo(offset);
}

@Override
protected String getOperation() {
return "External Modification (ago)";
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -30,4 +30,11 @@ public List<String> getIds() {
public void setIds(List<String> ids) {
this.ids = ids;
}

@Override
public String toString() {
StringBuilder writeOut = new StringBuilder();
writeOut.append("Action filter for action identifier: ").append(ids);
return writeOut.toString();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,4 +12,9 @@ public <F> F convert(
ActionFilterBuilder<F, ActionState, String, Instant, Long> filterBuilder, Stream<F> filters) {
return filterBuilder.or(filters);
}

@Override
protected String getOperation() {
return "OR";
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,9 @@ public class ActionFilterOrphaned extends ActionFilter {
public <F> F convert(ActionFilterBuilder<F, ActionState, String, Instant, Long> filterBuilder) {
return filterBuilder.orphaned();
}

@Override
public String toString() {
return "Action filter for orphaned actions";
}
}
Loading
Loading