Skip to content
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

RFC: connector deployment spec #94

Merged
merged 6 commits into from
Feb 13, 2024
Merged

RFC: connector deployment spec #94

merged 6 commits into from
Feb 13, 2024

Conversation

paf31
Copy link
Collaborator

@paf31 paf31 commented Feb 6, 2024

This is extracted from #89 in order to come to consensus on the aspects of packaging which are relevant only to deployment, while we figure out some of the specifics on the CLI side (watch mode etc.) in the existing RFC.

Rendered


Note: I reverted some of the changes from the other PR:

  • I think we agreed that that there is no harm in standardizing the config directory volume mount for all connectors, regardless of how they are built. I prefer this because it's simpler on the SDK side, and connectors can always choose not to use this directory.
  • I want to reserve fewer environment variable "namespaces", and env vars can always be remapped in Docker config anyway, so I'm reverting PORT to HASURA_CONNECTOR_PORT. The SDKs abstract this away from the user.

@paf31 paf31 marked this pull request as ready for review February 6, 2024 21:00
rfcs/0000-deployment.md Outdated Show resolved Hide resolved
rfcs/0000-deployment.md Outdated Show resolved Hide resolved
@daniel-chambers daniel-chambers mentioned this pull request Feb 7, 2024
2 tasks
- The following environment variables are reserved, and should not be used for connector-specific configuration:
- `HASURA_*`
- `OTEL_EXPORTER_*`
- Connectors can use environment variables as part of their configuration. Configuration that varies between different environments or regions (like connection strings) should be configurable via environment variables.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a "namespace" for these? Perhaps CONNECTOR_*?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not currently, but maybe there should be. I was actually thinking that we could remove any reserved variables by using command line args, and leave all environment variables available for use. But OpenTelemetry stops us from doing that, since they require env vars.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we can leave this for now and add it as a "best practice" if we feel the need.

@0x777
Copy link
Member

0x777 commented Feb 8, 2024

LGTM

Copy link
Collaborator

@daniel-chambers daniel-chambers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!


### Open Questions

- Do we want to reserve environment variables `OTEL_*` for possible future use of the [OTLP exporter spec](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/protocol/exporter.md)?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can avoid worrying about this and let connector devs suffer if they decide to intrude on another's namespace.

Fine with just pointing it out as a "probably don't do this".

@paf31 paf31 merged commit e0653f4 into main Feb 13, 2024
4 checks passed
@paf31 paf31 deleted the rfc/deployment-spec branch February 13, 2024 00:05
- `HASURA_*`
- `OTEL_EXPORTER_*`
- Connectors can use environment variables as part of their configuration. Configuration that varies between different environments or regions (like connection strings) should be configurable via environment variables.
- The connector should send any relevant trace spans in the OTLP format to the OTEL collector hosted at the URL provided by the `OTEL_EXPORTER_OTLP_ENDPOINT` environment variable.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be changed, per the spec the OTEL_EXPORTER_OTLP_ENDPOINT is a base url, and

For the per-signal variables (OTEL_EXPORTER_OTLP__ENDPOINT), the URL MUST be used as-is without any modification. The only exception is that if an URL contains no path part, the root path / MUST be used (see Example > 2).

If signals are sent that have no per-signal configuration from the previous point, OTEL_EXPORTER_OTLP_ENDPOINT is used as a base URL and the signals are sent to these paths relative to that:

Traces: v1/traces
Metrics: v1/metrics.
Logs: v1/logs.

So the URL for traces would be specified with either OTEL_EXPORTER_OTLP_TRACES_ENDPOINT or <OTEL_EXPORTER_OTLP_ENDPOINT>/v1/traces

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to the OTEL collector hosted at the URL provided by the OTEL_EXPORTER_OTLP_ENDPOINT

I think this is compatible with what the SDK does, and what the spec says? The OTEL collector root URL is provided in the OTEL_EXPORTER_OTLP_ENDPOINT variable. I will add a sentence clarifying this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants