-
Notifications
You must be signed in to change notification settings - Fork 19
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
Fix #177: Add APIs for Supplier<String> operation names #262
Conversation
Generate changelog in
|
Instead of continuing to deepen hacks it would be preferable to replace these unfortunate string concatenations with a presentation of the service name where a call takes place in our trace viewing application. That change would easily allow removal of the string munging since it would be obvious from the service boundaries where RPC calls are being made, and thus which side of the interface a particular call lives. |
I agree that would help in some scenarios, however it wouldn't solve the problem where we concatenate the verb and path in TraceEnrichingFilter or Tritium interface names. |
As far as I recall the monolithic services do not do a ton of string concatenation in their operation descriptions
I wish that were the case.
I’d continue to assert either way that the string concatenation is likely an issue that should be fixed
How would you fix this in the linked tritium handler?
…On Tue, Sep 10, 2019, at 17:38, Mark Elliot wrote:
As far as I recall the monolithic services do not do a ton of string concatenation in their operation descriptions, and I’d continue to assert either way that the string concatenation is likely an issue that should be fixed instead of continuing to jam hacks into this library.
________________________________
From: Carter Kozak ***@***.***>
Sent: Wednesday, September 11, 2019 1:33:30 AM
To: palantir/tracing-java ***@***.***>
Cc: Mark Elliot ***@***.***>; Review requested ***@***.***>
Subject: Re: [palantir/tracing-java] Fix #177: Add APIs for Supplier<String> operation names (#262)
I agree that would help in some scenarios, however it wouldn't solve the problem where we concatenate the verb and path in TraceEnrichingFilter [github.com]<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_palantir_tracing-2Djava_pull_262_files-23diff-2D648aa4dc0447e11e87cd638a98fc8dabL63&d=DwMCaQ&c=izlc9mHr637UR4lpLEZLFFS3Vn2UXBrZ4tFb6oOnmz8&r=Vp81aOxWZvuVgVK_wp-VF3pIYG92B19LcCw6XKeYC0U&m=PLA8O3zGYw3WYDBfc5v9jFcuVtoZpWWXpz-KYxC0xvY&s=Lzz65x8Sh-2I1_wsFf2ByjK-0d7RUE4QD9NgIV5F2yM&e=> or Tritium interface names [github.com]<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_palantir_tritium_blob_880c6da14f7d89644fce935a5fe0fb8e080b4586_tritium-2Dtracing_src_main_java_com_palantir_tritium_tracing_TracingInvocationEventHandler.java-23L70-2DL72&d=DwMCaQ&c=izlc9mHr637UR4lpLEZLFFS3Vn2UXBrZ4tFb6oOnmz8&r=Vp81aOxWZvuVgVK_wp-VF3pIYG92B19LcCw6XKeYC0U&m=PLA8O3zGYw3WYDBfc5v9jFcuVtoZpWWXpz-KYxC0xvY&s=Y-VACC3CcAKqE7SknfAc3K8bAd1P66cEyz2kC4MFK10&e=>.
You can imagine some of our services are large, and service boundaries aren't sufficient to provide tracing signal, so there tend to be several layers of traced services between endpoint, business logic, and persistence layers.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub [github.com]<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_palantir_tracing-2Djava_pull_262-3Femail-5Fsource-3Dnotifications-26email-5Ftoken-3DAAFHOQ33YMUWF6ELXTJGZRTQJA4FVA5CNFSM4IVNXQB2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6M4JMI-23issuecomment-2D530171057&d=DwMCaQ&c=izlc9mHr637UR4lpLEZLFFS3Vn2UXBrZ4tFb6oOnmz8&r=Vp81aOxWZvuVgVK_wp-VF3pIYG92B19LcCw6XKeYC0U&m=PLA8O3zGYw3WYDBfc5v9jFcuVtoZpWWXpz-KYxC0xvY&s=FkGDWEsTvO30efdkKTvOJ-ijdOsZao8uoqgh3Y0ssec&e=>, or mute the thread [github.com]<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_AAFHOQ6L6SG2IM55ZA2CR7DQJA4FVANCNFSM4IVNXQBQ&d=DwMCaQ&c=izlc9mHr637UR4lpLEZLFFS3Vn2UXBrZ4tFb6oOnmz8&r=Vp81aOxWZvuVgVK_wp-VF3pIYG92B19LcCw6XKeYC0U&m=PLA8O3zGYw3WYDBfc5v9jFcuVtoZpWWXpz-KYxC0xvY&s=sRGXlzVwCXeFm7_Abb9E-aIl8BkseuA4PV4IN5FWJSM&e=>.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub <#262?email_source=notifications&email_token=AA5M74P6VEUEDNNJ5N6Z6CLQJA4YPA5CNFSM4IVNXQB2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6M4QLA#issuecomment-530171948>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA5M74MZJQ6KMK2W3CNUKHDQJA4YPANCNFSM4IVNXQBQ>.
|
/** | ||
* Opens a new {@link SpanType#LOCAL LOCAL} span for this thread's call trace, labeled with the provided operation. | ||
*/ | ||
public static CloseableTracer startSpan(Supplier<String> operation) { |
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.
I'm worried that clients might choose to use this instead of the normal one for plain string concatenations which are not that expensive, thereby potentially slowing down hot paths.
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.
Today hot paths are slow depending on the length of the http path.
Before this PR
Hacks scattered across consumers to avoid expensive string work when the name will not be used, e.g. https://github.com/palantir/tritium/blob/880c6da14f7d89644fce935a5fe0fb8e080b4586/tritium-tracing/src/main/java/com/palantir/tritium/tracing/TracingInvocationEventHandler.java#L97-L103
After this PR
CloseableTracer.startSpan(() -> "Foo: " + bar)
Tracer.fastStartSpan(() -> "Foo: " + bar)
==COMMIT_MSG==
Add Supplier operation name APIs for lazy operation name evaluation
==COMMIT_MSG==