Implementation of OC-Agent Trace Service clients. #1476

songy23 · 2018-09-29T00:35:27Z

Equivalent to https://github.com/census-ecosystem/opencensus-go-exporter-ocagent/blob/master/ocagent.go.

This works similarly to Go Agent exporter:

Export will be handled in the main thread. OC library (SpanExporter) pushes SpanData to ExportClient, then ExportClient translates them to SpanProto and pushes to Agent.
Config will be handled in a daemon thread, since waiting and applying incoming Config will block the thread.

codecov-io · 2018-09-29T01:06:32Z

Codecov Report

Merging #1476 into master will decrease coverage by 0.88%.
The diff coverage is 26.61%.

@@             Coverage Diff              @@
##             master    #1476      +/-   ##
============================================
- Coverage     82.46%   81.57%   -0.89%     
- Complexity     1590     1595       +5     
============================================
  Files           243      244       +1     
  Lines          7493     7610     +117     
  Branches        702      718      +16     
============================================
+ Hits           6179     6208      +29     
- Misses         1098     1185      +87     
- Partials        216      217       +1

Impacted Files	Coverage Δ	Complexity Δ
...ter/trace/ocagent/OcAgentTraceExporterHandler.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...s/exporter/trace/ocagent/OcAgentTraceExporter.java	`22.58% <0%> (-1.56%)`	`3 <0> (ø)`
...census/exporter/trace/ocagent/TraceProtoUtils.java	`82.6% <0%> (-4.9%)`	`32 <0> (ø)`
...ace/ocagent/OcAgentTraceExporterConfiguration.java	`100% <100%> (ø)`	`2 <1> (ø)`	⬇️
...rter/trace/ocagent/OcAgentTraceServiceClients.java	`32.98% <32.98%> (ø)`	`3 <3> (?)`
...census/implcore/trace/export/SpanExporterImpl.java	`90.41% <0%> (-1.37%)`	`8% <0%> (ø)`
...encensus/contrib/dropwizard/DropWizardMetrics.java	`96.52% <0%> (-0.2%)`	`16% <0%> (ø)`
.../java/io/opencensus/metrics/export/TimeSeries.java	`100% <0%> (ø)`	`4% <0%> (+2%)`	⬆️
...ain/java/io/opencensus/implcore/metrics/Gauge.java	`100% <0%> (ø)`	`3% <0%> (ø)`	⬇️
... and 2 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 dea86ae...96011b2. Read the comment docs.

bogdandrutu

Some high-level comments about the overall design.

...agent/src/test/java/io/opencensus/exporter/trace/ocagent/OcAgentTraceServiceClientsTest.java

bogdandrutu · 2018-09-30T17:44:41Z

...e/ocagent/src/main/java/io/opencensus/exporter/trace/ocagent/OcAgentTraceServiceClients.java

+        }
+
+        @Override
+        public void onError(Throwable t) {}


should we at least log this? Should we try to reconnect?

Yes, this error is logged and we should retry on errors.

bogdandrutu · 2018-09-30T18:00:10Z

...e/ocagent/src/main/java/io/opencensus/exporter/trace/ocagent/OcAgentTraceServiceClients.java

+                "Failed to connect to OC-Agent because of %s. Retried %d time(s).", e, i));
+        // Exponential backoff.
+        try {
+          Thread.sleep(0, getBackoffTimeInNanos(i));


I think we should backoff with more than 1ms. Based on the definition of the Sleep the number of nanos has to be <1M.

I removed exponential backoff since we'll retry every 5 minutes (or a custom time interval).

bogdandrutu · 2018-09-30T18:10:14Z

.../ocagent/src/main/java/io/opencensus/exporter/trace/ocagent/OcAgentTraceExporterHandler.java

+    if (useInsecure == null) {
+      useInsecure = false;
+    }
+    client = OcAgentTraceServiceClients.openStreams(endPoint, useInsecure, serviceName);


Probably I miss understand the current design, but I want to make sure we are on the same page. I think the whole idea was to watch for the oc-agent to run. So if we fail to connect we should retry every 1min (or configurable time). The main workflow is if somebody deploys this then after 1 week they decide that they need traces they will be able to just deploy the ocagent without restarting their app.

High-level design:

In the application (or other library) we will have something like "registerOcAgentExporterWhenAvailable"

Start a new Thread (for the purpose of these explanation say name = ThreadOcAgent) that tries to connect to the oc-agent every 1 minute.

If not able to connect try again in 1 minute (or configurable time).

If able to connect:

Install the OcAgentSpanExportHandler to the SpanExporter. So spans will be exported.

When disconnected uninstall the OcAgentSpanExportHandler from the SpanExporter, also ThreadOcAgent should again retry to connect in 1 minute.

The config rpc handling can be done in the ThreadOcAgent as well.

So if we fail to connect we should retry every 1min (or configurable time).

Do you mean the Agent exporter should keep retrying forever if initiation did not succeed? I saw in Go we only attempted to connect 10 times with exponential backoff, that's also what I did here.

/cc @odeke-em

In the application (or other library) we will have something like "registerOcAgentExporterWhenAvailable"

There's already an API OcAgentTraceExporter.createAndRegister(). It registers the OcAgentTraceExporterHandler to SpanExporter. Before we managed to connect to Agent, the OcAgentTraceExporterHandler will just do no-op.

Start a new Thread (for the purpose of these explanation say name = ThreadOcAgent) that tries to connect to the oc-agent every 1 minute.

If not able to connect try again in 1 minute (or configurable time).

If we need to keep retrying connection, I agree this should be done in another thread.

Install the OcAgentSpanExportHandler to the SpanExporter. So spans will be exported.

When disconnected uninstall the OcAgentSpanExportHandler from the SpanExporter, also ThreadOcAgent should again retry to connect in 1 minute.

My current design is to register OcAgentSpanExportHandler first. If connection failed, it will just do no-op. I feel this is more straightforward and efficient than doing the register/unregister to SpanExporter every now and then, and these two approaches should have the same behavior to the end users.

The config rpc handling can be done in the ThreadOcAgent as well.

Config RPC has to be in its own thread, because Config is a blocking RPC. Once the client (OC-Library) sends back the CurrentLibraryConfig to Agent, that thread will be blocked waiting for the next UpdatedLibraryConfig.

Discussed offline, we think keep retrying is the right thing to do here. Will update the code based on that.

Sorry I hadn't seen this ping, I'll also create an issue for the Go agent exporter.

bogdandrutu · 2018-10-02T23:39:32Z

Please ping me when this is ready for review.

songy23 · 2018-10-05T16:45:47Z

Updated, PTAL. This works as follows:

In OcAgentTraceExporter.createAndRegister(), a OcAgentTraceExporterHandler will be created and registered to SpanExporter.
By the time when OcAgentTraceExporterHandler is created, a daemon thread AgentConnectionWorker will be created and started. This thread will be kept running forever. Every 5 minutes (or a custom time interval), it will try to connect to remote Agent.
If connection succeeded, SpanData passed to OcAgentTraceExporterHandler will be exported to Agent via RPC. AgentConnectionWorker will handle the UpdatedLibraryConfig.
If connection failed or lost, SpanData passed to OcAgentTraceExporterHandler will be ignored.

songy23 · 2018-10-09T17:12:13Z

Rebased, PTAL.

bogdandrutu · 2018-10-09T19:57:11Z

...e/ocagent/src/main/java/io/opencensus/exporter/trace/ocagent/OcAgentTraceServiceClients.java

+    return exportRequestObserverRef.get() != null;
+  }
+
+  private static boolean isConfigConnected() {


We should also have an option in the configuration of the ocagent exporter "enable_config" or something similar that allows users to configure if this exporter should also handle the config.

Also based on some comments on gitter we may have a configuration to that says keep connection open (for Android devices we may not want to always keep the RPC connection/request alive). We can address this later.

Added an option enableConfig to OcAgentTraceExporterConfiguration. Also left a TODO for adding an option on keeping the connection open.

Also renamed and re-organized a few helper methods.

bogdandrutu

Some comments that I wrote early this morning. I will do a full review soon.

bogdandrutu · 2018-10-10T17:26:07Z

build.gradle

@@ -26,6 +26,10 @@ def useCheckerFramework = rootProject.hasProperty('checkerFramework')
 def useErrorProne = !useCheckerFramework

 subprojects {
+    // OC-Agent exporter depends on grpc-core which depends on errorprone annotations, and it
+    // doesn't wort with checker framework.


s/wort/work

bogdandrutu · 2018-10-10T17:26:44Z

...nt/src/main/java/io/opencensus/exporter/trace/ocagent/OcAgentTraceExporterConfiguration.java

+  /**
+   * Returns the retry time interval when trying to connect to Agent.
+   *
+   * @return the retry time interval


"." at the end?

bogdandrutu · 2018-10-10T17:27:29Z

...nt/src/main/java/io/opencensus/exporter/trace/ocagent/OcAgentTraceExporterConfiguration.java

+   * @since 0.17
+   */
+  @Nullable
+  public abstract Boolean getEnableConfig();


use boolean and make the default value "true"

songy23 · 2018-10-11T21:27:28Z

As we discussed, I'll try to decouple the logic and send separate PRs. Closing this PR for now.

songy23 added tracing exporters labels Sep 29, 2018

songy23 assigned bogdandrutu Sep 29, 2018

songy23 requested a review from bogdandrutu September 29, 2018 00:35

googlebot added the cla: yes label Sep 29, 2018

songy23 added this to the Release 0.17.0 milestone Sep 29, 2018

bogdandrutu suggested changes Sep 30, 2018

View reviewed changes

songy23 assigned songy23 and unassigned bogdandrutu Sep 30, 2018

songy23 changed the title ~~Implementation of OC-Agent Trace Service clients.~~ [WIP] Implementation of OC-Agent Trace Service clients. Oct 2, 2018

songy23 added the action required The pull request is blocked by something other than a need for code review. label Oct 2, 2018

songy23 mentioned this pull request Oct 2, 2018

trace/exporters/ocagent: Implement retry logic and keep retrying in a daemon thread. census-instrumentation/opencensus-python#333

Open

songy23 changed the title ~~[WIP] Implementation of OC-Agent Trace Service clients.~~ Implementation of OC-Agent Trace Service clients. Oct 5, 2018

songy23 removed the action required The pull request is blocked by something other than a need for code review. label Oct 5, 2018

songy23 mentioned this pull request Oct 5, 2018

Testing: Add a fake implementation of TraceService. #1481

Merged

songy23 assigned bogdandrutu and unassigned songy23 Oct 5, 2018

songy23 added 5 commits October 9, 2018 10:08

Implementation of OC-Agent Trace Service clients.

b1a2ed6

Update errorprone.

f090fec

Remove duplication.

f341547

Allow retry interval to be customized.

eca1915

Have a daemon thread that runs forever and tries to connect to Agent.

68141ab

songy23 force-pushed the agent-exporter-impl branch from 78c3ce7 to 68141ab Compare October 9, 2018 17:11

bogdandrutu reviewed Oct 9, 2018

View reviewed changes

songy23 added 2 commits October 9, 2018 14:23

Add an option on enabling config to configuration.

c2df7c1

Don't block the thread if config is not enabled.

db5397b

Also renamed and re-organized a few helper methods.

bogdandrutu reviewed Oct 10, 2018

View reviewed changes

songy23 added 2 commits October 10, 2018 13:44

Fix review comments.

080143a

Remove unnecessary Nullable.

96011b2

This was referenced Oct 10, 2018

Exporter/OcAgent: Add options on retry interval and config. #1487

Merged

Exporter/OcAgent: Add methods for getting and applying config. #1488

Merged

songy23 added the DO NOT MERGE label Oct 11, 2018

songy23 closed this Oct 11, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation of OC-Agent Trace Service clients. #1476

Implementation of OC-Agent Trace Service clients. #1476

songy23 commented Sep 29, 2018 •

edited

Loading

codecov-io commented Sep 29, 2018 •

edited

Loading

bogdandrutu left a comment

bogdandrutu Sep 30, 2018

songy23 Oct 5, 2018

bogdandrutu Sep 30, 2018

songy23 Oct 5, 2018

bogdandrutu Sep 30, 2018

songy23 Oct 1, 2018

songy23 Oct 2, 2018

odeke-em Oct 2, 2018

bogdandrutu commented Oct 2, 2018

songy23 commented Oct 5, 2018

songy23 commented Oct 9, 2018

bogdandrutu Oct 9, 2018

songy23 Oct 9, 2018

bogdandrutu left a comment

bogdandrutu Oct 10, 2018

songy23 Oct 10, 2018

bogdandrutu Oct 10, 2018

songy23 Oct 10, 2018

bogdandrutu Oct 10, 2018

songy23 Oct 10, 2018

songy23 commented Oct 11, 2018

Implementation of OC-Agent Trace Service clients. #1476

Implementation of OC-Agent Trace Service clients. #1476

Conversation

songy23 commented Sep 29, 2018 • edited Loading

codecov-io commented Sep 29, 2018 • edited Loading

Codecov Report

bogdandrutu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bogdandrutu commented Oct 2, 2018

songy23 commented Oct 5, 2018

songy23 commented Oct 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bogdandrutu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

songy23 commented Oct 11, 2018

songy23 commented Sep 29, 2018 •

edited

Loading

codecov-io commented Sep 29, 2018 •

edited

Loading