-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Source Datadog: Update CDK version and dependencies, remove parameters and migrate to inline schemas #44371
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great work, but it's broken ;) I think the fix is very easy.
Again, strong signal for me that we need to make the test reports public.
Here's the failure that CI shows at connector build step:
process "python /airbyte/integration_code/main.py spec" did not complete successfully: exit code: 1
Stdout:
{"type":"LOG","log":{"level":"FATAL","message":"Object of type datetime is not JSON serializable\nTraceback (most recent call last):\n File \"/airbyte/integration_code/main.py\", line 8, in <module>\n run()\n File \"/airbyte/integration_code/source_datadog/run.py\", line 14, in run\n launch(source, sys.argv[1:])\n File \"/usr/local/lib/python3.10/site-packages/airbyte_cdk/entrypoint.py\", line 234, in launch\n for message in source_entrypoint.run(parsed_args):\n File \"/usr/local/lib/python3.10/site-packages/airbyte_cdk/entrypoint.py\", line 97, in run\n source_spec: ConnectorSpecification = self.source.spec(self.logger)\n File \"/usr/local/lib/python3.10/site-packages/airbyte_cdk/sources/declarative/manifest_declarative_source.py\", line 142, in spec\n self._emit_manifest_debug_message(extra_args={\"source_name\": self.name, \"parsed_config\": json.dumps(self._source_config)})\n File \"/usr/local/lib/python3.10/json/__init__.py\", line 231, in dumps\n return _default_encoder.encode(obj)\n File \"/usr/local/lib/python3.10/json/encoder.py\", line 199, in encode\n chunks = self.iterencode(o, _one_shot=True)\n File \"/usr/local/lib/python3.10/json/encoder.py\", line 257, in iterencode\n return _iterencode(o, 0)\n File \"/usr/local/lib/python3.10/json/encoder.py\", line 179, in default\n raise TypeError(f'Object of type {o.__class__.__name__} '\nTypeError: Object of type datetime is not JSON serializable"}}
{"type":"TRACE","trace":{"type":"ERROR","emitted_at":1724003967406.657,"error":{"message":"Something went wrong in the connector. See the logs for more details.","internal_message":"Object of type datetime is not JSON serializable","stack_trace":"Traceback (most recent call last):\n File \"/airbyte/integration_code/main.py\", line 8, in <module>\n run()\n File \"/airbyte/integration_code/source_datadog/run.py\", line 14, in run\n launch(source, sys.argv[1:])\n File \"/usr/local/lib/python3.10/site-packages/airbyte_cdk/entrypoint.py\", line 234, in launch\n for message in source_entrypoint.run(parsed_args):\n File \"/usr/local/lib/python3.10/site-packages/airbyte_cdk/entrypoint.py\", line 97, in run\n source_spec: ConnectorSpecification = self.source.spec(self.logger)\n File \"/usr/local/lib/python3.10/site-packages/airbyte_cdk/sources/declarative/manifest_declarative_source.py\", line 142, in spec\n self._emit_manifest_debug_message(extra_args={\"source_name\": self.name, \"parsed_config\": json.dumps(self._source_config)})\n File \"/usr/local/lib/python3.10/json/__init__.py\", line 231, in dumps\n return _default_encoder.encode(obj)\n File \"/usr/local/lib/python3.10/json/encoder.py\", line 199, in encode\n chunks = self.iterencode(o, _one_shot=True)\n File \"/usr/local/lib/python3.10/json/encoder.py\", line 257, in iterencode\n return _iterencode(o, 0)\n File \"/usr/local/lib/python3.10/json/encoder.py\", line 179, in default\n raise TypeError(f'Object of type {o.__class__.__name__} '\nTypeError: Object of type datetime is not JSON serializable\n","failure_type":"system_error","stream_descriptor":null}}}
Stderr:
requests-mock = "^1.9.3" | ||
pytest = "^6.2" | ||
pytest-mock = "^3.6.1" | ||
requests-mock = "*" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
python = "^3.9,<3.12" | ||
airbyte-cdk = "^1" | ||
python = "^3.10,<3.12" | ||
airbyte-cdk = "^4" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feeling adventurous I see
@@ -5,6 +5,8 @@ acceptance_tests: | |||
spec: | |||
tests: | |||
- spec_path: "source_datadog/spec.yaml" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You removed the spec file, so the spec path here should lead to the manifest, I think. /cc @ChristoGrab
@@ -5,6 +5,8 @@ acceptance_tests: | |||
spec: | |||
tests: | |||
- spec_path: "source_datadog/spec.yaml" | |||
backward_compatibility_tests_config: | |||
disable_for_version: "0.4.15" # Set default start and end date for incremental sync |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory, you only want to disable the backwards compatibility tests if the new version will not work, but the old one worked.
If the new version provides defaults and works in a scenario when a new one does not, why would it be broken?
Also, usually if you're skipping backwards compat checks, this should signal that the change is breaking, which this is not.
BUT, your build step in CI is failing on something something date can't be parsed something, so seems relevant.
/cc @jnr0790 |
Oops, I've now converted the default start and end date to string, it should work now. |
@natikgadzhi The breaking change is -> Added default start date and end date to the spec |
pattern: ^[0-9]{4}-[0-9]{2}-[0-9]{2}T[0-9]{2}:[0-9]{2}:[0-9]{2}Z$ | ||
examples: | ||
- "2022-10-01T00:00:00Z" | ||
default: "2024-12-01T00:00:00Z" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, that would mean that the start date by default is in the future.
What if instead of setting the default, we make this field optional, and use a srtring interpolation to default to 1 year ago, but dynamically?
Similar to this:
datetime: |
@girarda what's a better practice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed my mind: let's perhaps default to a specific start date, but in the past — smaller change. 2024-01-01 maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments on start date default value, and suggestions on breaking change messaging.
Leaving as open so @btkcodedev you can catch up with these and ask questions / apply / figure out start date default values.
pattern: ^[0-9]{4}-[0-9]{2}-[0-9]{2}T[0-9]{2}:[0-9]{2}:[0-9]{2}Z$ | ||
examples: | ||
- "2022-10-01T00:00:00Z" | ||
default: "2024-12-01T00:00:00Z" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed my mind: let's perhaps default to a specific start date, but in the past — smaller change. 2024-01-01 maybe?
Co-authored-by: Natik Gadzhi <natik@respawn.io>
Done |
👏🏼 |
@jnr0790 this is merged! It's a breaking change, so once it lands, follow up with the customer and tell them they'd need to opt-in. |
What
Closes #41965
How