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

ENG-52474: copy all hypertrace config to traceable #29

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

prodion23
Copy link
Contributor

@prodion23 prodion23 commented Oct 29, 2024

This takes the hypertrace/agent-config protobuf & tools and adds them here instead. It was a pretty clean cut path except for JavaAgent field(both hypertrace & traceable) had that field so they are merged here.

The resulting protobuf is the same as before, it's just a single config object instead of 1 hypertrace & 1 traceable.

Why do this?
(I didn't want to)

  • We can't(at least I cannot) publish pythonagent to pypi anymore - so we have to move all our instrumentation from hypertrace/python agent to traceable/python agent, this also is where the hypertrace config lives s

Alternatives:

  • copy instrumentation from hypertrace/lang-agent to traceable/lang-agent (as part of deprecating hypertrace)
  • clone hypertrace/agent-config into the traceable/lang-agent, build both tracebale & hypertrace protos there, add config loading code to traceable from hypertrace, traceable/lang-agent will maintain both configs.
  • continue to support loading 2 configs, this also means hypertrace/agent-config needs to stick around

Lint problem:

proto/ai/traceable/agent/config/v1/config.proto:291:3:Enum value name "B3" should be prefixed with "PROPAGATION_FORMAT_".
... (9 more, view Action output

I worry if we don't ignore these lint warnings we'll introduce breaking change if anyone was setting those fields via env vars^ are we okay adding the following to the any preexisting enums to maintain backwards compat?
// protolint:disable:next ENUM_FIELD_NAMES_PREFIX

@varkey98
Copy link
Contributor

Just a note to myself, there's some merging of messages I wanted to do when we did this merge. Its the metrics field. Both hypertrace and traceable configs have it.

@varkey98
Copy link
Contributor

but since we can't publish hypertrace/python-agent any more I can't get the hypertrace config into Traceable/pythonagent easily. So something has to change either way.

Does this mean that we are completely stopping to release hypertrace agents?

@prodion23
Copy link
Contributor Author

Just a note to myself, there's some merging of messages I wanted to do when we did this merge. Its the metrics field. Both hypertrace and traceable configs have it.
@varkey98 I can merge them here. Is it this from Hypertrace:

// Telemetry has config for agent telemetry: traces and metrics on agent's
// performance and events.
message Telemetry {
    // when `true`, an internal span is created and exported when the agent is initialized and started.
    // It's useful to denote when the application the agent is in started.
    google.protobuf.BoolValue startup_span_enabled = 1;

    // Whether to capture metrics or not. The metrics will be otel go metrics.
    // See https://github.com/open-telemetry/opentelemetry-go/tree/main/metric
    google.protobuf.BoolValue metrics_enabled = 2;
}

into this:

message MetricsConfig {
    // set this flag to enable metrics
    google.protobuf.BoolValue enabled = 1;
    // endpoint level configuration
    EndpointMetricsConfig endpoint_config = 2;
    // config for printing metrics in logs
    MetricsLogConfig logging = 3;
}

(if not just lmk)

@varkey98
Copy link
Contributor

@prodion23 Yes, but pls hold on that merge, will do it in a separate pr. Will ask everyone's opinion as well before doing that :)

@tim-mwangi
Copy link
Contributor

@prodion23 do we want to keep all the generated code or do we still leave it to the client repos to generate it?

@prodion23
Copy link
Contributor Author

@tim-mwangi I'm not super opinionated since python/node/ruby have been generating the code on their own anyway.

For Go maybe it is a convenience item to keep them here? (would defer to you & others opinion if we want to keep it or not)

My understanding is now all of the hypertrace config should be getting generated as part of config_pb.go so on go agent you'd still be able to import the same? (it would just be the config wiring needs updated)

@tim-mwangi
Copy link
Contributor

@tim-mwangi I'm not super opinionated since python/node/ruby have been generating the code on their own anyway.

For Go maybe it is a convenience item to keep them here? (would defer to you & others opinion if we want to keep it or not)

My understanding is now all of the hypertrace config should be getting generated as part of config_pb.go so on go agent you'd still be able to import the same? (it would just be the config wiring needs updated)

I think we can keep it as it has been. Just generate for golang in the repo and the other languages can keep generating on their own.

NONE = 4;

// OTLP over http
OTLP_HTTP = 5;
Copy link
Contributor

Choose a reason for hiding this comment

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

We can avoid buf linting these in the buf.yml file. I think the config is like this

ignore_only:
    ENUM_ZERO_VALUE_SUFFIX:
      - config/common/v1/config.proto
    ENUM_VALUE_PREFIX:
      - config/common/v1/config.proto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried that(a few iterations of it) but that doesn't appear to work for at least our version(I see it referenced in the v1 and v2 docs but unsure of v1beta1) I used except instead which I think these configs would be analogous unless we add a new file.

func main() {
var outDir = flag.String("out", ".", "Out directory for the generated code.")
var optModule = flag.String("opt-module", ".", "Module for the generated code.")
var envPrefix = flag.String("env-prefix", "HT_", "Prefix for env var loading")
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we keep this around or do we want to change to TA? Backwards compatibility could force us to keep it around.

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.

3 participants