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

Add new formatting function "add" #2507

Conversation

drewcorlin1
Copy link
Contributor

@drewcorlin1 drewcorlin1 commented Dec 1, 2024

Which problem is this PR solving?

In addition to #2501 and #2504, allowing any numbers (in my immediate case a date in microseconds since epoch) to be offset by a number is useful to allow users to see logs before/after their span/trace timestamps by a configurable offset.

Description of the changes

This adds a add function and gives the ability to chain formatting functions together (eg #{endTime | add 60000000 | epoch_micros_to_date_iso})

How was this change tested?

Unit tests and with the following UI config

const DEFAULT_CONFIG = {
  "linkPatterns": [
    {
      "type": "traces",
      "url": "https://myKibana/_dashboards/app/discover#/?_g=(time:(from:'#{startTime | shift_epoch_micros -60000000 | epoch_micros_to_date_iso}',to:'#{endTime | shift_epoch_micros 60000000 | epoch_micros_to_date_iso}'))&_a=(columns:!(_source),filters:!(('$state':(store:appState),meta:(alias:!n,disabled:!f,index:filebeat-all,key:json.traceId,negate:!f,params:(query:'42c7668cd8d887da'),type:phrase),query:(match_phrase:(json.traceId:'42c7668cd8d887da')))),index:filebeat-all)",
      "text": "Logs"
    }
  ]
};

Checklist

@drewcorlin1 drewcorlin1 requested a review from a team as a code owner December 1, 2024 15:10
@drewcorlin1 drewcorlin1 requested review from albertteoh and removed request for a team December 1, 2024 15:10
@drewcorlin1 drewcorlin1 force-pushed the shift-epoch-micros-and-format-function-chaining branch 4 times, most recently from 2a6307c to a7f3493 Compare December 1, 2024 16:07
Copy link

codecov bot commented Dec 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.63%. Comparing base (a083195) to head (0aa811b).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2507   +/-   ##
=======================================
  Coverage   96.62%   96.63%           
=======================================
  Files         255      255           
  Lines        7707     7719   +12     
  Branches     2003     2004    +1     
=======================================
+ Hits         7447     7459   +12     
  Misses        260      260           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@yurishkuro
Copy link
Member

As described the use case doesn't make sense to me. How does an operator of Jaeger install know what possible time skew might exist?

I can see the other use case: you want to see the logs from +-1min of the span timestamp, and need to transform the ts into an interval. If that's what you meant please update the description.

As far as implementation, can this be simply an "add" function? It does not care about the units (up to the user), and by allowing negative arguments it can adjust ts in either direction.

@drewcorlin1
Copy link
Contributor Author

I can see the other use case: you want to see the logs from +-1min of the span timestamp, and need to transform the ts into an interval. If that's what you meant please update the description.

Yes this is what I meant. Will update.

Good point about it just being add. Will rename to be more generic

@drewcorlin1 drewcorlin1 changed the title Shift epoch micros and format function chaining "add" formatting function and format function chaining Dec 1, 2024
Signed-off-by: Drew Corlin <[email protected]>
@drewcorlin1 drewcorlin1 force-pushed the shift-epoch-micros-and-format-function-chaining branch 2 times, most recently from 8e2ef57 to 95ae9bd Compare December 1, 2024 17:39
packages/jaeger-ui/src/utils/link-formatting.tsx Outdated Show resolved Hide resolved
packages/jaeger-ui/src/utils/link-formatting.tsx Outdated Show resolved Hide resolved
packages/jaeger-ui/src/utils/link-formatting.tsx Outdated Show resolved Hide resolved
packages/jaeger-ui/src/utils/link-formatting.tsx Outdated Show resolved Hide resolved
packages/jaeger-ui/src/utils/link-formatting.tsx Outdated Show resolved Hide resolved
packages/jaeger-ui/src/utils/link-formatting.tsx Outdated Show resolved Hide resolved
}
return (val: T) => formatFunction(val, ...args);
})
.filter((fn): fn is NonNullable<typeof fn> => fn !== null);
Copy link
Member

Choose a reason for hiding this comment

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

odd statement, NonNullable is compared against null?

I think you could use flatMap() instead and return [] in cases of errors such that it will be automatically skipped

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a type predicate which says that it returns a boolean indicating if fn is non-null. It may make more sense reading it with a curly brace-wrapped function body

.filter((fn): fn is NonNullable<typeof fn> => {
  return fn !== null;
});

I could use flatMap, but would just add more allocations for the same behavior right?

packages/jaeger-ui/src/utils/link-formatting.tsx Outdated Show resolved Hide resolved
packages/jaeger-ui/src/utils/link-formatting.tsx Outdated Show resolved Hide resolved
@drewcorlin1 drewcorlin1 force-pushed the shift-epoch-micros-and-format-function-chaining branch 2 times, most recently from 2ede5ab to e8a2944 Compare December 1, 2024 20:47
@drewcorlin1 drewcorlin1 force-pushed the shift-epoch-micros-and-format-function-chaining branch from e8a2944 to 0aa811b Compare December 1, 2024 20:50
@drewcorlin1
Copy link
Contributor Author

I don't think I can add labels. @yurishkuro would you be able to add the "changelog:bugfix-or-minor-feature" label (or whatever you deem appropriate)

@yurishkuro yurishkuro added the changelog:bugfix-or-minor-feature 🐞 Bug fixes, Minor Improvements label Dec 1, 2024
@yurishkuro yurishkuro changed the title "add" formatting function and format function chaining Add new formatting function "add" Dec 1, 2024
@yurishkuro yurishkuro merged commit 9c286c2 into jaegertracing:main Dec 1, 2024
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:bugfix-or-minor-feature 🐞 Bug fixes, Minor Improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants