Emit job and dataset runless metadata #1880

pawel-big-lebowski · 2023-05-25T08:23:14Z

Problem

Proposal: https://github.com/OpenLineage/OpenLineage/blob/main/proposals/1837/static_lineage.md

Closes: #1868

Solution

Add proposed event types to spec,
Include client test,
Include support in python client,
Verify event encoding in Marquez (to be done)

Note: All schema changes require discussion. Please link the issue for context.

Your change modifies the core OpenLineage model
Your change modifies one or more OpenLineage facets

If you're contributing a new integration, please specify the scope of the integration and how/where it has been tested (e.g., Apache Spark integration supports S3 and GCS filesystem operations, tested with AWS EMR).

One-line summary:

Checklist

You've signed-off your work
Your pull request title follows our guidelines
Your changes are accompanied by tests (if relevant)
Your change contains a small diff and is self-contained
You've updated any relevant documentation (if relevant)
Your comment includes a one-liner for the changelog about the specific purpose of the change (if necessary)
You've versioned the core OpenLineage model or facets according to SchemaVer (if relevant)
You've added a header to source files (if relevant)

SPDX-License-Identifier: Apache-2.0
Copyright 2018-2023 contributors to the OpenLineage project

pawel-big-lebowski · 2023-05-26T06:15:36Z

spec/OpenLineage.json

+      "properties": {
+        "dataset": {
+          "allOf": [
+            { "$ref": "#/$defs/Dataset" }


This is kind of workaround ;)

So, current code generation for Java works in a way such that base classes are generated as interfaces and only top level objects are classes. Dataset is only an interface while InputDataset and OutputDataset is a class. So, we cannot use Dataset directly here, instead we create an object with allOf containing Dataset. This advantage of this is that the existing code of generated client does not get modified. The disadvantage is a method name newDatasetEventDataset to create an instance of DatasetEventDataset as in case of un-named objects, generated class name is a parent name concatenated with a property name.

maybe rename Dataset to BaseDataset and then add a Dataset type that "extends" BaseDataset?

I'm not sure what's best.

Or create a named StaticDataset type to use here as a ref

I will try StaticDataset approach.

codecov-commenter · 2023-05-26T07:43:43Z

Codecov Report

Merging #1880 (f1a4af2) into main (b928e2e) will not change coverage.
The diff coverage is n/a.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff            @@
##               main    #1880   +/-   ##
=========================================
  Coverage     81.92%   81.92%           
  Complexity      100      100           
=========================================
  Files            85       85           
  Lines          3586     3586           
  Branches         27       27           
=========================================
  Hits           2938     2938           
  Misses          617      617           
  Partials         31       31

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

mobuchowski · 2023-05-30T12:16:08Z

client/java/src/main/java/io/openlineage/client/transports/ConsoleTransport.java

@@ -20,4 +20,16 @@ public void emit(OpenLineage.RunEvent runEvent) {
    // if DEBUG loglevel is enabled, this will double-log even due to OpenLineageClient also logging
    log.info(OpenLineageClientUtils.toJson(runEvent));
  }
+
+  @Override
+  public void emit(OpenLineage.DatasetEvent datasetEvent) {


My overcomplicating mind suggests me stuff like

public <E extends OpenLineage.Event> void emit(E datasetEvent) {

...but I don't thing it's a good idea.

To make it work, we would need to introduce Event class/interface within a code that is generated from spec. To avoid hardcoding, this should have been included within spec. So the json-schema would not only contain data definition, but also some hints on how to implement model classes.

The code generation will create a base class if you use allOf like in facets:
https://github.com/OpenLineage/OpenLineage/blob/main/spec/facets/ColumnLineageDatasetFacet.json#L5-L8
but I'm not sure that it is necessary.

mobuchowski · 2023-05-30T12:16:47Z

client/java/src/main/java/io/openlineage/client/transports/Transport.java

@@ -23,4 +23,12 @@ enum Type {
  Transport(@NonNull final Type type) {}

  public abstract void emit(OpenLineage.RunEvent runEvent);
+
+  public void emit(OpenLineage.DatasetEvent datasetEvent) {


Why not abstract? Do not want to enforce implementation in Transports made for previous versions?

Users may have custom transport implementations. Making it abstract would enforce implementation on their side and would break their code even though they may have no interest in static metadata.

should we update the transport api to add a lower level void emit(String jsonEvent) ?

What would be the usage of emit(String)? Our clients define models that make it clear the types allowed and enforces a structure. Allowing for string type allows for flexibility, but introduces the likelihood of invalid events being emitted.

mobuchowski · 2023-06-01T12:43:17Z

🚀 🚀 🚀

julienledem

I added a few cosmetic comments (and a couple questions) but overall this looks great.
Please let me know which of those comments you would be considering adding.

julienledem · 2023-06-02T16:15:00Z

client/java/generator/src/main/java/io/openlineage/client/TypeResolver.java

+          List<ResolvedType> resolvedTypes = oneOfType
+              .getTypes()
+              .stream()
+              .filter(type -> type instanceof RefType)


I would throw an exception when there's another type than the ones you expect (RefType). Maybe just map with a cast to throw if not?

That's kind of workaround for:

"oneOf": [ { "$ref": "#/$defs/RunEvent" }, { "$ref": "#/$defs/DatasetEvent" }, { "$ref": "#/$defs/JobEvent" } ]

We don't want to generate single Java class to handle that (may be undoable), but we want to generate code for each of types which is RefType. The method returns the first of the resolved types because it has to align with

public ResolvedType visit(...) {

contract and changing contract to a list or optional would require changes across a whole code generator.

julienledem · 2023-06-02T16:54:14Z

client/java/src/main/java/io/openlineage/client/transports/ConsoleTransport.java

@@ -20,4 +20,16 @@ public void emit(OpenLineage.RunEvent runEvent) {
    // if DEBUG loglevel is enabled, this will double-log even due to OpenLineageClient also logging
    log.info(OpenLineageClientUtils.toJson(runEvent));
  }
+
+  @Override
+  public void emit(OpenLineage.DatasetEvent datasetEvent) {


The code generation will create a base class if you use allOf like in facets:
https://github.com/OpenLineage/OpenLineage/blob/main/spec/facets/ColumnLineageDatasetFacet.json#L5-L8
but I'm not sure that it is necessary.

julienledem · 2023-06-02T17:00:58Z

client/java/src/main/java/io/openlineage/client/transports/HttpTransport.java

+  @Override
+  public void emit(@NonNull OpenLineage.DatasetEvent datasetEvent) {
+    final String eventAsJson = OpenLineageClientUtils.toJson(datasetEvent);
+    emit(eventAsJson);


nit but I'd do a static import and just emit(toJson(datasetEvent)) for those 3 methods

julienledem · 2023-06-02T17:02:09Z

client/java/src/main/java/io/openlineage/client/transports/Transport.java

@@ -23,4 +23,12 @@ enum Type {
  Transport(@NonNull final Type type) {}

  public abstract void emit(OpenLineage.RunEvent runEvent);
+
+  public void emit(OpenLineage.DatasetEvent datasetEvent) {


should we update the transport api to add a lower level void emit(String jsonEvent) ?

client/java/src/test/java/io/openlineage/client/Events.java

julienledem · 2023-06-02T20:59:34Z

spec/OpenLineage.json

+      "properties": {
+        "dataset": {
+          "allOf": [
+            { "$ref": "#/$defs/Dataset" }


maybe rename Dataset to BaseDataset and then add a Dataset type that "extends" BaseDataset?

julienledem · 2023-06-02T20:59:48Z

spec/OpenLineage.json

+      "properties": {
+        "dataset": {
+          "allOf": [
+            { "$ref": "#/$defs/Dataset" }


I'm not sure what's best.

julienledem · 2023-06-02T23:08:38Z

spec/OpenLineage.json

+      "properties": {
+        "dataset": {
+          "allOf": [
+            { "$ref": "#/$defs/Dataset" }


Or create a named StaticDataset type to use here as a ref

julienledem · 2023-06-02T23:11:13Z

spec/OpenLineage.json

+          "description": "The JSON Pointer (https://tools.ietf.org/html/rfc6901) URL to the corresponding version of the schema definition for this RunEvent",
+          "type": "string",
+          "format": "uri",
+          "example": "https://openlineage.io/spec/2-0-0/OpenLineage.json#/$defs/DatasetEvent"


I'm assuming this is how we decide what type of event this is.

yes, the approach has been tested within Marquez PR -> MarquezProject/marquez#2495

julienledem · 2023-06-02T23:12:32Z

spec/OpenLineage.json

+        "eventTime",
+        "dataset",
+        "producer",
+        "schemaURL"


maybe eventTime, producer and schemaURL are part of a base Event type added with a AllOf like we do for Input/OutputDatasets and facets.

This is cool! I will try it out.

julienledem

I added a few cosmetic comments (and a couple questions) but overall this looks great.
Please let me know which of those comments you would be considering adding.

Signed-off-by: Pawel Leszczynski <leszczynski.pawel@gmail.com>

wslulciuc

Minor comments, but otherwise LGTM 💯 🥇

Signed-off-by: Pawel Leszczynski <leszczynski.pawel@gmail.com>

pawel-big-lebowski · 2023-06-06T10:47:55Z

@julienledem Superb comments!

I've created StaticDataset
I've introduced BaseEvent with schemaUrl, eventTime and producer fields
I've reduced amount of emit methods by adding emit(String)

pawel-big-lebowski added the area:spec Specifications and standards for the project label May 25, 2023

boring-cyborg bot added the area:client/java openlineage-java label May 25, 2023

pawel-big-lebowski force-pushed the spec/runless-events branch from 90a1967 to 07e0915 Compare May 25, 2023 11:28

boring-cyborg bot added the area:client/python openlineage-python label May 25, 2023

pawel-big-lebowski force-pushed the spec/runless-events branch from 07e0915 to 27c2f15 Compare May 25, 2023 12:25

pawel-big-lebowski commented May 26, 2023

View reviewed changes

pawel-big-lebowski force-pushed the spec/runless-events branch 3 times, most recently from 29257e3 to 62a11ab Compare May 26, 2023 07:17

pawel-big-lebowski force-pushed the spec/runless-events branch from 62a11ab to f1a4af2 Compare May 29, 2023 07:38

boring-cyborg bot added the area:documentation Improvements or additions to documentation label May 29, 2023

pawel-big-lebowski requested review from mobuchowski and JDarDagran May 29, 2023 11:21

pawel-big-lebowski force-pushed the spec/runless-events branch from f1a4af2 to 9bd8d8a Compare May 29, 2023 12:12

boring-cyborg bot added the area:integration/spark label May 29, 2023

pawel-big-lebowski force-pushed the spec/runless-events branch 6 times, most recently from 4f8c177 to 5677917 Compare May 30, 2023 07:34

pawel-big-lebowski marked this pull request as ready for review May 30, 2023 07:56

pawel-big-lebowski mentioned this pull request May 30, 2023

Ability to decode static metadata events MarquezProject/marquez#2495

Merged

7 tasks

mobuchowski reviewed May 30, 2023

View reviewed changes

JDarDagran approved these changes May 30, 2023

View reviewed changes

pawel-big-lebowski force-pushed the spec/runless-events branch from 5677917 to dda0d85 Compare June 1, 2023 10:47

mobuchowski approved these changes Jun 1, 2023

View reviewed changes

julienledem approved these changes Jun 2, 2023

View reviewed changes

pawel-big-lebowski force-pushed the spec/runless-events branch from dda0d85 to f2524a3 Compare June 6, 2023 08:10

static metadata

219cae7

Signed-off-by: Pawel Leszczynski <leszczynski.pawel@gmail.com>

pawel-big-lebowski force-pushed the spec/runless-events branch from f2524a3 to a8b5040 Compare June 6, 2023 08:37

wslulciuc approved these changes Jun 6, 2023

View reviewed changes

include BaseEvent class

186496a

Signed-off-by: Pawel Leszczynski <leszczynski.pawel@gmail.com>

pawel-big-lebowski force-pushed the spec/runless-events branch from a8b5040 to 186496a Compare June 6, 2023 09:31

pawel-big-lebowski mentioned this pull request Jun 6, 2023

document OL 2.0.0 OpenLineage/docs#175

Open

pawel-big-lebowski merged commit cc3a71e into main Jun 7, 2023

pawel-big-lebowski deleted the spec/runless-events branch June 7, 2023 06:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Emit job and dataset runless metadata #1880

Emit job and dataset runless metadata #1880

pawel-big-lebowski commented May 25, 2023 •

edited

Loading

pawel-big-lebowski May 26, 2023

julienledem Jun 2, 2023

julienledem Jun 2, 2023

julienledem Jun 2, 2023

pawel-big-lebowski Jun 5, 2023

codecov-commenter commented May 26, 2023 •

edited

Loading

mobuchowski May 30, 2023

pawel-big-lebowski May 30, 2023

julienledem Jun 2, 2023

mobuchowski May 30, 2023

pawel-big-lebowski May 31, 2023

julienledem Jun 2, 2023

wslulciuc Jun 6, 2023

mobuchowski commented Jun 1, 2023

julienledem left a comment

julienledem Jun 2, 2023

pawel-big-lebowski Jun 5, 2023

julienledem Jun 2, 2023

julienledem Jun 2, 2023

julienledem Jun 2, 2023

julienledem Jun 2, 2023

julienledem Jun 2, 2023

julienledem Jun 2, 2023

julienledem Jun 2, 2023

pawel-big-lebowski Jun 6, 2023

julienledem Jun 2, 2023

pawel-big-lebowski Jun 6, 2023

julienledem left a comment

wslulciuc left a comment

pawel-big-lebowski commented Jun 6, 2023

Emit job and dataset runless metadata #1880

Emit job and dataset runless metadata #1880

Conversation

pawel-big-lebowski commented May 25, 2023 • edited Loading

Problem

Solution

One-line summary:

Checklist

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

codecov-commenter commented May 26, 2023 • edited Loading

Codecov Report

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

mobuchowski commented Jun 1, 2023

julienledem 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

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

julienledem left a comment

Choose a reason for hiding this comment

wslulciuc left a comment

Choose a reason for hiding this comment

pawel-big-lebowski commented Jun 6, 2023

pawel-big-lebowski commented May 25, 2023 •

edited

Loading

codecov-commenter commented May 26, 2023 •

edited

Loading