Skip to content
This repository has been archived by the owner on Mar 27, 2021. It is now read-only.

Improve tracing and logging for write errors. #740

Merged
merged 2 commits into from
Jan 12, 2021

Conversation

hexedpackets
Copy link
Contributor

This adds error flags and exception messages to the spans for
metadata, suggest, and bigtable writes when the chain of futures
fails. The same exceptions are also logged. Previously some
exceptions, such as grpc errors, could be masked by sl4j settings.

The trace for a metric write was cleaned up to remove several
intermediary spans. These spans did not have useful information and
had no branching paths, so they were just clutter in the overall
trace.

Closes #724.

This adds error flags and exception messages to the spans for
metadata, suggest, and bigtable writes when the chain of futures
fails. The same exceptions are also logged. Previously some
exceptions, such as grpc errors, could be masked by sl4j settings.

The trace for a metric write was cleaned up to remove several
intermediary spans. These spans did not have useful information and
had no branching paths, so they were just clutter in the overall
trace.

Closes #724.
@hexedpackets hexedpackets requested a review from a team January 8, 2021 22:45
@codecov
Copy link

codecov bot commented Jan 8, 2021

Codecov Report

Merging #740 (5f30b9a) into master (037a871) will increase coverage by 0.20%.
The diff coverage is 66.66%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #740      +/-   ##
============================================
+ Coverage     54.82%   55.02%   +0.20%     
- Complexity     3105     3130      +25     
============================================
  Files           742      743       +1     
  Lines         20191    20248      +57     
  Branches       1313     1316       +3     
============================================
+ Hits          11070    11142      +72     
+ Misses         8653     8633      -20     
- Partials        468      473       +5     
Impacted Files Coverage Δ Complexity Δ
...spotify/heroic/consumer/pubsub/PubSubConsumer.java 50.00% <ø> (ø) 2.00 <0.00> (ø)
...va/com/spotify/heroic/metric/MetricCollection.java 66.66% <ø> (+3.50%) 12.00 <0.00> (ø)
.../com/spotify/heroic/metric/LocalMetricManager.java 64.58% <0.00%> (-1.49%) 4.00 <0.00> (ø)
...ic/metric/bigtable/api/BigtableDataClientImpl.java 20.77% <0.00%> (ø) 4.00 <0.00> (ø)
...istics/semantic/SemanticMetricBackendReporter.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...om/spotify/heroic/consumer/schemas/Spotify100.java 51.78% <37.14%> (+0.89%) 4.00 <0.00> (ø)
...roic/metadata/elasticsearch/MetadataBackendKV.java 80.66% <42.85%> (-1.08%) 50.00 <1.00> (ø)
...heroic/suggest/elasticsearch/SuggestBackendKV.java 81.48% <50.00%> (-0.77%) 77.00 <1.00> (ø)
...potify/heroic/metric/bigtable/BigtableBackend.java 85.81% <59.37%> (-3.12%) 61.00 <3.00> (ø)
...fy/heroic/aggregation/ComputeDistributionStat.java 64.28% <64.28%> (ø) 1.00 <1.00> (?)
... and 20 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 20e3327...5f30b9a. Read the comment docs.

Copy link
Contributor

@ao2017 ao2017 left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -82,7 +83,7 @@
* See: https://github.com/spotify/heroic/pull/208 */
protected boolean brokenSegmentsPr208 = false;
protected boolean eventSupport = false;
protected Optional<Integer> maxBatchSize = Optional.empty();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the replacement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just doing some cleanup as I went through the code. While I generally like Optional, in this case its only purpose is a single assume statement in the test, so removing it makes things a tiny bit easier to read/less verbose.

@hexedpackets hexedpackets merged commit 1b45699 into master Jan 12, 2021
@delete-merged-branch delete-merged-branch bot deleted the write-error-visibility branch January 12, 2021 18:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Heroic Bigtable Consumer does not handle failures as expected
3 participants