Hi Jing! > the upcoming OpenTelemetry based TraceReporter will use the same Span > implementation and will not support trace_id and span_id. Does it make > sense to at least add the span_id into the current Span design? The default > implementation could follow your suggestion: jobId#attemptId#checkpointId.
Those IDs (jobId, checkpointId) will be accessible to the humans via attributes, so there is no need to encode them at the moment in the span/trace ids. At the same time, at the moment, I don't know for sure how the concept of span parent ids should be exposed to the user of this API. Whether it should be plain text, or some pojo generating the trace id/span id. Also I'm not sure how would this have to work for other reporting systems other than OTEL. Due to those reasons I thought that keeping the API as simple as possible would be the best option. > 1. The sample code shows that the scope of Span will be the CanonicalName > of a class. If there are other cases that could be used as the scope too, a > javadoc about Span scope would be helpful. If the CanonicalName of a class > is the best practice, removing the scope from the builder constructor and > adding setScope(Class) might ease the API usage. The Span.getScope() can > still return String. I like the idea with `#setScope(Class)`. I will adjust the FLIP :) > 2. The sample code in the FLIP is not consistent. The first example used > Span.builder(..) and the second example used new Span() with setters. I will fix that, I've forgotten to upgrade the second `new Span()` usage to the builder. > 3. I guess the constructor of SpanBuilder class is a typo. Yes! Thanks for noting. Best, Piotrek czw., 16 lis 2023 o 15:12 Roman Khachatryan <ro...@apache.org> napisał(a): > Thanks for the proposal, > > Starting with the minimal functionality and expanding if necessary as the > FLIP describes makes a lot of sense to me. > > Regards, > Roman > > On Wed, Nov 15, 2023, 9:31 PM Jing Ge <j...@ververica.com.invalid> wrote: > > > Hi Piotr, > > > > Sorry for the late reply and thanks for the proposal, it looks awesome! > > > > In the discussion, you pointed out that it is difficult to build true > > distributed traces. afaiu from FLIP-384 and FLIP-385, the > > upcoming OpenTelemetry based TraceReporter will use the same Span > > implementation and will not support trace_id and span_id. Does it make > > sense to at least add the span_id into the current Span design? The > default > > implementation could follow your suggestion: > jobId#attemptId#checkpointId. > > > > Some other NIT questions: > > 1. The sample code shows that the scope of Span will be the CanonicalName > > of a class. If there are other cases that could be used as the scope > too, a > > javadoc about Span scope would be helpful. If the CanonicalName of a > class > > is the best practice, removing the scope from the builder constructor and > > adding setScope(Class) might ease the API usage. The Span.getScope() can > > still return String. > > 2. The sample code in the FLIP is not consistent. The first example used > > Span.builder(..) and the second example used new Span() with setters. > > 3. I guess the constructor of SpanBuilder class is a typo. > > > > Really a nice idea to introduce the trace report! Thanks again! > > > > Best regards, > > Jing > > > > On Tue, Nov 14, 2023 at 3:16 PM Piotr Nowojski <pnowoj...@apache.org> > > wrote: > > > > > Hi All, > > > > > > Thanks for the answers! > > > > > > Unless there are some objections or suggestions, I will open a voting > > > thread later this > > > week. > > > > > > > My original thought was to show how much time a sampled record is > > > processed > > > > within each operator in stream processing. By saying 'sampled', I > mean > > we > > > > won't generate a trace for every record due to the high cost > involved. > > > > Instead, we could only trace ONE record from source when the user > > > requests > > > > it (via REST API or Web UI) or when triggered periodically at a very > > low > > > > frequency. > > > > > > That would be useful, but another issue is that we can not measure time > > > reliably at the > > > granularity of a single record. Time to process a single record by the > > > whole operator > > > chain is usually faster compared to the syscalls to measure time. > > > > > > So I think we are stuck with sample based profilers, like Flame Graphs > > > generated by > > > the Flink WebUI. > > > > > > Best, Piotrek > > > > > > czw., 9 lis 2023 o 05:32 Rui Fan <1996fan...@gmail.com> napisał(a): > > > > > > > Hi Piotr: > > > > > > > > Thanks for your reply! > > > > > > > > > About structured logging (basically events?) I vaguely remember > some > > > > > discussions about that. It might be a much larger topic, so I would > > > > prefer > > > > > to leave it out of the scope of this FLIP. > > > > > > > > Sounds make sense to me! > > > > > > > > > I think those could be indeed useful. If you would like to > contribute > > > to > > > > them > > > > > in the future, I would be happy to review the FLIP for it :) > > > > > > > > Thank you, after this FLIP, I or my colleagues can pick it up! > > > > > > > > Best, > > > > Rui > > > > > > > > On Thu, Nov 9, 2023 at 11:39 AM Zakelly Lan <zakelly....@gmail.com> > > > wrote: > > > > > > > > > Hi Piotr, > > > > > > > > > > Thanks for your detailed explanation! I could see the challenge of > > > > > implementing traces with multiple spans and agree to put it in the > > > future > > > > > work. I personally prefer the idea of generating multi span traces > > for > > > > > checkpoints on the JM only. > > > > > > > > > > > I'm not sure if I understand the proposal - I don't know how > traces > > > > could > > > > > > be used for this purpose? > > > > > > Traces are perfect for one of events (like checkpointing, > recovery, > > > > etc), > > > > > > not for continuous monitoring > > > > > > (like processing records). That's what metrics are. Creating > trace > > > and > > > > > > span(s) per each record would > > > > > > be prohibitively expensive. > > > > > > > > > > My original thought was to show how much time a sampled record is > > > > processed > > > > > within each operator in stream processing. By saying 'sampled', I > > mean > > > we > > > > > won't generate a trace for every record due to the high cost > > involved. > > > > > Instead, we could only trace ONE record from source when the user > > > > requests > > > > > it (via REST API or Web UI) or when triggered periodically at a > very > > > low > > > > > frequency. However after re-thinking my idea, I realized it's hard > to > > > > > define the whole lifecycle of a record since it is transformed into > > > > > different forms among operators. We could discuss this in future > > after > > > > the > > > > > basic trace is implemented in Flink. > > > > > > > > > > > Unless you mean in batch/bounded jobs? Then yes, we could create > a > > > > > bounded > > > > > > job trace, with spans > > > > > > for every stage/task/subtask. > > > > > > > > > > Oh yes, batch jobs could definitely leverage the trace. > > > > > > > > > > Best, > > > > > Zakelly > > > > > > > > > > > > > > > On Wed, Nov 8, 2023 at 9:18 PM Jinzhong Li < > lijinzhong2...@gmail.com > > > > > > > > wrote: > > > > > > > > > > > Hi Piotr, > > > > > > > > > > > > Thanks for driving this proposal! I strongly agree that the > > > existing > > > > > > metric APIs are not suitable for monitoring restore/checkpoint > > > > behavior! > > > > > > > > > > > > I think the TM-level recovery/checkpointing traces are necessary > in > > > the > > > > > > future. In our production environment, we sometimes encounter > that > > > job > > > > > > recovery time is very long (30min+), due to several subTask heavy > > > disk > > > > > > traffic. The TM-level recovery trace is helpful for > troubleshooting > > > > such > > > > > > issues. > > > > > > > > > > > > Best > > > > > > Jinzhong > > > > > > > > > > > > On Wed, Nov 8, 2023 at 5:09 PM Piotr Nowojski < > > pnowoj...@apache.org> > > > > > > wrote: > > > > > > > > > > > > > Hi Zakelly, > > > > > > > > > > > > > > Thanks for the comments. Quick answer for both of your > questions > > > > would > > > > > be > > > > > > > that it probably should be > > > > > > > left as a future work. For more detailed answers please take a > > look > > > > > below > > > > > > > :) > > > > > > > > > > > > > > > Does it mean the inclusion and subdivision relationships of > > spans > > > > > > defined > > > > > > > > by "parent_id" are not supported? I think it is a very > > necessary > > > > > > feature > > > > > > > > for the trace. > > > > > > > > > > > > > > Yes exactly, that is the current limitation. This could be > solved > > > > > somehow > > > > > > > one way or another in the future. > > > > > > > > > > > > > > Support for reporting multi span traces all at once - for > example > > > > > > > `CheckpointStatsTracker` running JM, > > > > > > > could upon checkpoint completion create in one place the whole > > > > > structure > > > > > > of > > > > > > > parent spans, to have for > > > > > > > example one span per each subtask. This would be a relatively > > easy > > > > > follow > > > > > > > up. > > > > > > > > > > > > > > However, if we would like to create true distributed traces, > with > > > > spans > > > > > > > reported from many different > > > > > > > components, potentially both on JM and TM, the problem is a bit > > > > deeper. > > > > > > The > > > > > > > issue in that case is how > > > > > > > to actually fill out `parrent_id` and `trace_id`? Passing some > > > > context > > > > > > > entity as a java object would be > > > > > > > unfeasible. That would require too many changes in too many > > > places. I > > > > > > think > > > > > > > the only realistic way > > > > > > > to do it, would be to have a deterministic generator of > > `parten_id` > > > > and > > > > > > > `trace_id` values. > > > > > > > > > > > > > > For example we could create the parent trace/span of the > > checkpoint > > > > on > > > > > > JM, > > > > > > > and set those ids to > > > > > > > something like: `jobId#attemptId#checkpointId`. Each subtask > then > > > > could > > > > > > > re-generate those ids > > > > > > > and subtasks' checkpoint span would have an id of > > > > > > > `jobId#attemptId#checkpointId#subTaskId`. > > > > > > > Note that this is just an example, as most likely distributed > > spans > > > > for > > > > > > > checkpointing do not make > > > > > > > sense, as we can generate them much easier on the JM anyway. > > > > > > > > > > > > > > > In addition to checkpoint and recovery, I believe the trace > > would > > > > > also > > > > > > be > > > > > > > > valuable for performance tuning. If Flink can trace and > > visualize > > > > the > > > > > > > time > > > > > > > > cost of each operator and stage for a sampled record, users > > would > > > > be > > > > > > able > > > > > > > > to easily determine the end-to-end latency and identify > > > performance > > > > > > > issues > > > > > > > > for optimization. Looking forward to seeing these in the > > future. > > > > > > > > > > > > > > I'm not sure if I understand the proposal - I don't know how > > traces > > > > > could > > > > > > > be used for this purpose? > > > > > > > Traces are perfect for one of events (like checkpointing, > > recovery, > > > > > etc), > > > > > > > not for continuous monitoring > > > > > > > (like processing records). That's what metrics are. Creating > > trace > > > > and > > > > > > > span(s) per each record would > > > > > > > be prohibitively expensive. > > > > > > > > > > > > > > Unless you mean in batch/bounded jobs? Then yes, we could > create > > a > > > > > > bounded > > > > > > > job trace, with spans > > > > > > > for every stage/task/subtask. > > > > > > > > > > > > > > Best, > > > > > > > Piotrek > > > > > > > > > > > > > > > > > > > > > śr., 8 lis 2023 o 05:30 Zakelly Lan <zakelly....@gmail.com> > > > > > napisał(a): > > > > > > > > > > > > > > > Hi Piotr, > > > > > > > > > > > > > > > > Happy to see the trace! Thanks for this proposal. > > > > > > > > > > > > > > > > One minor question: It is mentioned in the interface of Span: > > > > > > > > > > > > > > > > Currently we don't support traces with multiple spans. Each > > span > > > is > > > > > > > > > self-contained and represents things like a checkpoint or > > > > recovery. > > > > > > > > > > > > > > > > > > > > > > > > Does it mean the inclusion and subdivision relationships of > > spans > > > > > > defined > > > > > > > > by "parent_id" are not supported? I think it is a very > > necessary > > > > > > feature > > > > > > > > for the trace. > > > > > > > > > > > > > > > > In addition to checkpoint and recovery, I believe the trace > > would > > > > > also > > > > > > be > > > > > > > > valuable for performance tuning. If Flink can trace and > > visualize > > > > the > > > > > > > time > > > > > > > > cost of each operator and stage for a sampled record, users > > would > > > > be > > > > > > able > > > > > > > > to easily determine the end-to-end latency and identify > > > performance > > > > > > > issues > > > > > > > > for optimization. Looking forward to seeing these in the > > future. > > > > > > > > > > > > > > > > Best, > > > > > > > > Zakelly > > > > > > > > > > > > > > > > > > > > > > > > On Tue, Nov 7, 2023 at 6:27 PM Piotr Nowojski < > > > > pnowoj...@apache.org> > > > > > > > > wrote: > > > > > > > > > > > > > > > > > Hi Rui, > > > > > > > > > > > > > > > > > > Thanks for the comments! > > > > > > > > > > > > > > > > > > > 1. I see the trace just supports Span? Does it support > > trace > > > > > > events? > > > > > > > > > > I'm not sure whether tracing events is reasonable for > > > > > > TraceReporter. > > > > > > > > > > If it supports, flink can report checkpoint and > checkpoint > > > path > > > > > > > > > proactively. > > > > > > > > > > Currently, checkpoint lists or the latest checkpoint can > > only > > > > be > > > > > > > > fetched > > > > > > > > > > by external components or platforms. And report is more > > > timely > > > > > and > > > > > > > > > > efficient than fetch. > > > > > > > > > > > > > > > > > > No, currently the `TraceReporter` that I'm introducing > > supports > > > > > only > > > > > > > > single > > > > > > > > > span traces. > > > > > > > > > So currently neither events on their own, nor events inside > > > spans > > > > > are > > > > > > > not > > > > > > > > > supported. > > > > > > > > > This is done just for the sake of simplicity, and test out > > the > > > > > basic > > > > > > > > > functionality. But I think, > > > > > > > > > those currently missing features should be added at some > > point > > > in > > > > > > > > > the future. > > > > > > > > > > > > > > > > > > About structured logging (basically events?) I vaguely > > remember > > > > > some > > > > > > > > > discussions about > > > > > > > > > that. It might be a much larger topic, so I would prefer to > > > leave > > > > > it > > > > > > > out > > > > > > > > of > > > > > > > > > the scope of this > > > > > > > > > FLIP. > > > > > > > > > > > > > > > > > > > 2. This FLIP just monitors the checkpoint and task > > recovery, > > > > > right? > > > > > > > > > > > > > > > > > > Yes, it only adds single span traces for checkpointing and > > > > > > > > > recovery/initialisation - one > > > > > > > > > span per whole job per either recovery/initialization > process > > > or > > > > > per > > > > > > > each > > > > > > > > > checkpoint. > > > > > > > > > > > > > > > > > > > Could we add more operations in this FLIP? In our > > production, > > > > we > > > > > > > > > > added a lot of trace reporters for job starts and > scheduler > > > > > > > operation. > > > > > > > > > > They are useful if some jobs start slowly, because they > > will > > > > > affect > > > > > > > > > > the job availability. For example: > > > > > > > > > > - From JobManager process is started to JobGraph is > created > > > > > > > > > > - From JobGraph is created to JobMaster is created > > > > > > > > > > - From JobMaster is created to job is running > > > > > > > > > > - From start request tm from yarn or kubernetes to all > tms > > > are > > > > > > ready > > > > > > > > > > - etc > > > > > > > > > > > > > > > > > > I think those could be indeed useful. If you would like to > > > > > contribute > > > > > > > > them > > > > > > > > > in the future, > > > > > > > > > I would be happy to review the FLIP for it :) > > > > > > > > > > > > > > > > > > > Of course, this FLIP doesn't include them is fine for me. > > The > > > > > first > > > > > > > > > version > > > > > > > > > > only initializes the interface and common operations, and > > we > > > > can > > > > > > add > > > > > > > > > > more operations in the future > > > > > > > > > > > > > > > > > > Yes, that's exactly my thinking :) > > > > > > > > > > > > > > > > > > Best, > > > > > > > > > Piotrek > > > > > > > > > > > > > > > > > > wt., 7 lis 2023 o 10:05 Rui Fan <1996fan...@gmail.com> > > > > napisał(a): > > > > > > > > > > > > > > > > > > > Hi Piotr, > > > > > > > > > > > > > > > > > > > > Thanks for driving this proposal! The trace reporter is > > > useful > > > > to > > > > > > > > > > check a lot of duration monitors inside of Flink. > > > > > > > > > > > > > > > > > > > > I have some questions about this proposal: > > > > > > > > > > > > > > > > > > > > 1. I see the trace just supports Span? Does it support > > trace > > > > > > events? > > > > > > > > > > I'm not sure whether tracing events is reasonable for > > > > > > TraceReporter. > > > > > > > > > > If it supports, flink can report checkpoint and > checkpoint > > > path > > > > > > > > > > proactively. > > > > > > > > > > Currently, checkpoint lists or the latest checkpoint can > > only > > > > be > > > > > > > > fetched > > > > > > > > > > by external components or platforms. And report is more > > > timely > > > > > and > > > > > > > > > > efficient than fetch. > > > > > > > > > > > > > > > > > > > > 2. This FLIP just monitors the checkpoint and task > > recovery, > > > > > right? > > > > > > > > > > Could we add more operations in this FLIP? In our > > production, > > > > we > > > > > > > > > > added a lot of trace reporters for job starts and > scheduler > > > > > > > operation. > > > > > > > > > > They are useful if some jobs start slowly, because they > > will > > > > > affect > > > > > > > > > > the job availability. For example: > > > > > > > > > > - From JobManager process is started to JobGraph is > created > > > > > > > > > > - From JobGraph is created to JobMaster is created > > > > > > > > > > - From JobMaster is created to job is running > > > > > > > > > > - From start request tm from yarn or kubernetes to all > tms > > > are > > > > > > ready > > > > > > > > > > - etc > > > > > > > > > > > > > > > > > > > > Of course, this FLIP doesn't include them is fine for me. > > The > > > > > first > > > > > > > > > version > > > > > > > > > > only initializes the interface and common operations, and > > we > > > > can > > > > > > add > > > > > > > > > > more operations in the future. > > > > > > > > > > > > > > > > > > > > Best, > > > > > > > > > > Rui > > > > > > > > > > > > > > > > > > > > On Tue, Nov 7, 2023 at 4:31 PM Piotr Nowojski < > > > > > > pnowoj...@apache.org> > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > Hi all! > > > > > > > > > > > > > > > > > > > > > > I would like to start a discussion on FLIP-384: > Introduce > > > > > > > > TraceReporter > > > > > > > > > > and > > > > > > > > > > > use it to create checkpointing and recovery traces [1]. > > > > > > > > > > > > > > > > > > > > > > This proposal intends to improve observability of > Flink's > > > > > > > > Checkpointing > > > > > > > > > > and > > > > > > > > > > > Recovery/Initialization operations, by adding support > for > > > > > > reporting > > > > > > > > > > traces > > > > > > > > > > > from Flink. In the future, reporting traces can be of > > > course > > > > > used > > > > > > > for > > > > > > > > > > other > > > > > > > > > > > use cases and also by users. > > > > > > > > > > > > > > > > > > > > > > There are also two other follow up FLIPS, FLIP-385 [2] > > and > > > > > > FLIP-386 > > > > > > > > > [3], > > > > > > > > > > > which expand the basic functionality introduced in > > FLIP-384 > > > > > [1]. > > > > > > > > > > > > > > > > > > > > > > Please let me know what you think! > > > > > > > > > > > > > > > > > > > > > > Best, > > > > > > > > > > > Piotr Nowojski > > > > > > > > > > > > > > > > > > > > > > [1] > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/FLINK/FLIP-384%3A+Introduce+TraceReporter+and+use+it+to+create+checkpointing+and+recovery+traces > > > > > > > > > > > [2] > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/FLINK/FLIP-385%3A+Add+OpenTelemetryTraceReporter+and+OpenTelemetryMetricReporter > > > > > > > > > > > [3] > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/FLINK/FLIP-386%3A+Support+adding+custom+metrics+in+Recovery+Spans > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
