Uh oh!
There was an error while loading. Please reload this page.
PHOENIX-5215 opentelemetry changes replacing htrace - #1282
Open
kiran-maturi wants to merge 14 commits into
Open
Conversation
stoty
commented
Aug 5, 2021
Contributor
💔 -1 overall
This message was automatically generated. |
stoty
commented
Aug 5, 2021
Contributor
💔 -1 overall
This message was automatically generated. |
stoty
commented
Aug 16, 2021
Contributor
💔 -1 overall
This message was automatically generated. |
richardantal
left a comment
Contributor
There was a problem hiding this comment.
Nice Progress!
I left some comments to fix in the next commit.
| <groupId>org.apache.htrace</groupId> | ||
| <artifactId>htrace-core</artifactId> | ||
| <groupId>io.opentelemetry</groupId> | ||
| <artifactId>opentelemetry-api</artifactId> |
Contributor
There was a problem hiding this comment.
Already defined in line 404
| <dependency> | ||
| <groupId>io.opentelemetry</groupId> | ||
| <artifactId>opentelemetry-context</artifactId> | ||
| <version>${opentelemetry.version}</version> |
Contributor
There was a problem hiding this comment.
version should be in the dependencyManagement
| Tracer.getInstance().deliver(span); | ||
| assertTrue("Span never committed to table", latch.await(30, TimeUnit.SECONDS)); | ||
| //Tracer.getInstance().deliver(span); |
Contributor
There was a problem hiding this comment.
These lines should be deleted
| Span span = TraceUtil.getGlobalTracer().spanBuilder("Creating basic query for " + getPlanSteps(iterator)).startSpan(); | ||
| if (span.isRecording()) return new TracingIterator(span, iterator); |
Contributor
There was a problem hiding this comment.
Is it going to be true anytime?
ContributorAuthor
There was a problem hiding this comment.
@richardantal default implementation shows it as false. I will take a look at it in depth and see how we can enable TracingIterator
| * @since 0.1 | ||
| */ | ||
| public final class PhoenixDriver extends PhoenixEmbeddedDriver { | ||
| public final class PhoenixDriver extends PhoenixEmbeddedDriver { |
| import org.apache.phoenix.schema.types.PDataType; | ||
| import org.apache.phoenix.schema.types.PLong; | ||
| import org.apache.phoenix.schema.types.PVarchar; | ||
| import org.apache.phoenix.trace.TraceUtil; |
| // create the phoenix connection | ||
| Properties props = new Properties(); | ||
| props.setProperty(QueryServices.TRACING_FREQ_ATTRIB, Tracing.Frequency.NEVER.getKey()); | ||
| //props.setProperty(QueryServices.TRACING_FREQ_ATTRIB, Tracing.Frequency.NEVER.getKey()); |
ContributorAuthor
There was a problem hiding this comment.
@richardantal I will remove this once I have made changes to handle tracing at connection
| import org.apache.phoenix.iterate.ResultIterator; | ||
| import org.apache.phoenix.schema.tuple.Tuple; | ||
| import org.apache.htrace.TraceScope; | ||
| import org.apache.htrace.HTraceConfiguration; | ||
| import io.opentelemetry.api.trace.Span; | ||
| import io.opentelemetry.api.trace.Tracer; | ||
| import io.opentelemetry.context.Scope; |
Contributor
There was a problem hiding this comment.
i think it can be deleted, as it is not used.
| import org.apache.phoenix.jdbc.PhoenixConnection; | ||
| import org.apache.phoenix.parse.TraceStatement; | ||
| import org.apache.phoenix.query.QueryServices; | ||
| import org.apache.phoenix.job.JobManager; |
dbwong
reviewed
Oct 18, 2021
| import io.opentelemetry.context.Scope; | ||
| import org.apache.phoenix.trace.TraceUtil; | ||
| import java.sql.*; |
Contributor
There was a problem hiding this comment.
nit: Phoenix style guidelines does not want to use import *
| import java.sql.*; | ||
| public class LocalConnectionTest { |
Contributor
There was a problem hiding this comment.
Is this class assuming a local instance is running? Can this be made an actual IT?
ContributorAuthor
There was a problem hiding this comment.
@dbwong this test was written for local testing i have removed it.
| import java.nio.file.Paths; | ||
| import java.util.List; | ||
| public class TestFSShellCluster { |
Contributor
There was a problem hiding this comment.
Same with this class on testing and IT
ContributorAuthor
There was a problem hiding this comment.
@dbwong this was also written as part of local testing. have removed it
| if (child != null) { | ||
| child.stop(); | ||
| } | ||
| child.end(); |
Contributor
There was a problem hiding this comment.
What did we change here that insure child is non-null?
ContributorAuthor
There was a problem hiding this comment.
@dbwong In general opentelemetry default implementation would return an invalid span (when telemetry is not enabled) or a valid span same goes for startSpan as well. some other implementation might return null. I will add the check again
| context.getOverallQueryMetrics().updateNumParallelScans(numScans); | ||
| GLOBAL_NUM_PARALLEL_SCANS.update(numScans); | ||
| final long renewLeaseThreshold = context.getConnection().getQueryServices().getRenewLeaseThresholdMilliSeconds(); | ||
| //TODO: Parent Span creation using Span.current() and create children with description "Parallel scanner for table: " + tableRef.getTable().getPhysicalName().getString() |
Contributor
There was a problem hiding this comment.
Is this being handled in this JIRA or a followup?
ContributorAuthor
There was a problem hiding this comment.
@dbwong I am planning to handle this in this JIRA.
| @Override | ||
| public PeekingResultIterator call() throws Exception { | ||
| //TODO: create child span for each call using parent |
Contributor
There was a problem hiding this comment.
This one we use to handle with Tracing.wrap right?
ContributorAuthor
There was a problem hiding this comment.
@dbwong yes i had updated the implementation. was seeing issues so had to disable it. Will take a look at it again
| public class TraceReader { | ||
| private static final Logger LOGGER = LoggerFactory.getLogger(TraceReader.class); | ||
| public static final long ROOT_SPAN_ID = 0x74ace; |
Contributor
There was a problem hiding this comment.
Is this the recommended approach for a top level span?
ContributorAuthor
There was a problem hiding this comment.
@dbwong I am not sure if we need TraceReader anymore, considering we can view the traces on exporter ui (like zipking or jaeger). I wanted to discuss about TracingWebApplication as well in the JIRA will comment there
kiran-maturi
commented
Jan 18, 2022
ContributorAuthor
@virajjasani@richardantal can you please review |
kiran-maturi
marked this pull request as ready for review
July 28, 2022 10:41
gjacoby126
commented
Aug 19, 2022
Contributor
@kiran-maturi - I'm curious what the status of this PR is (needs a rebase at the least), and if we're trying to get it into the upcoming Phoenix 5.2 release? |
kiran-maturi
commented
Oct 25, 2022
ContributorAuthor
@gjacoby126 sorry for the late response. I have rebased it working on fixing other issues as well. Will keep you posted |
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.