Skip to content

Add backward compatibility configs for tally to otel migration - #7235

Merged
yycptt merged 3 commits into
temporalio:mainfrom
yycptt:otel-sec
Feb 6, 2025
Merged

Add backward compatibility configs for tally to otel migration#7235
yycptt merged 3 commits into
temporalio:mainfrom
yycptt:otel-sec

Conversation

@yycptt

@yycptt yycptt commented Feb 4, 2025

Copy link
Copy Markdown
Member

What changed?

  • Add backward compatibility configs for tally to otel migration

Why?

  • There are some breaking changes when switching from tally to otel metric framework.
  1. Unit name will be appended to the metric name
  2. _total will be appended to counter metrics
  3. Timer metric will be emitted as milliseconds instead of seconds
    This PR adds configs for controlling both behaviors when using otel framework so that the behavior will be the same as tally.

How did you test it?

  • Unit tests
  • Tested locally with various combination of configurations.

Potential risks

  • No risk by default. If new options are enabled, timer metric value may be wrong or not bucketed correctly.

Documentation

Is hotfix candidate?

  • No.

@yycptt
yycptt requested review from bergundy and yiminc February 4, 2025 19:08
@yycptt

yycptt commented Feb 4, 2025

Copy link
Copy Markdown
Member Author

Related to #7211

@bergundy bergundy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, looks like there's good test coverage.

@yycptt
yycptt marked this pull request as ready for review February 5, 2025 01:39
@yycptt
yycptt requested a review from a team as a code owner February 5, 2025 01:39
Comment thread common/metrics/config.go
WithoutUnitSuffix bool `yaml:"withoutUnitSuffix"`
// WithoutCounterSuffix controls the additional of _total suffixes to counter metric names.
// This config only takes effect when using opentelemetry framework.
WithoutCounterSuffix bool `yaml:"withoutCounterSuffix"`

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Also added config for disabling the addition of _total to Counters

@yycptt
yycptt merged commit af09a38 into temporalio:main Feb 6, 2025
@yycptt
yycptt deleted the otel-sec branch February 6, 2025 01:31
@1nu

1nu commented Feb 7, 2025

Copy link
Copy Markdown

Hi! I might be wrong, but to me it seems like OTEL is missing backwards compatibility for tally users that configured prefix.

@yycptt

yycptt commented Feb 7, 2025

Copy link
Copy Markdown
Member Author

Hi! I might be wrong, but to me it seems like OTEL is missing backwards compatibility for tally users that configured prefix.

Hi @1nu, thanks for reporting!

Yeah, looks like it's the case and Prefix is never supported when framework is set to opentelementry. I will look into how to keep the behavior the same for tally users.

@yycptt

yycptt commented Feb 7, 2025

Copy link
Copy Markdown
Member Author

Support Prefix in OTEL: #7297

@1nu

1nu commented Feb 8, 2025

Copy link
Copy Markdown

I just discovered another potential backward compatibility issue in handling tags keys and values with dashes, reporting here just in case. Tally replaces dashes with underscores in both key and value, opentelemetry only replaces dashes in the key.
For example:

...
    tags:
      temporal-cluster: "order-management"
...

becomes temporal_cluster="order_management" using tally and temporal_cluster="order-management" with opentelemetry.

@yycptt

yycptt commented Feb 15, 2025

Copy link
Copy Markdown
Member Author

I have reverted the change to always use OTEL as the framework, so there will be no impact for Tally users in the next release.

We will look into those issues and do some more testing. Also needs to continue to support statsD users somehow with otel before fully deprecate tally.

Thanks again @1nu for flagging those issues!

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.

4 participants