taoensso / telemere

Structured telemetry library for Clojure/Script
https://www.taoensso.com/telemere
Eclipse Public License 1.0
200 stars 5 forks source link

tel/trace! doesn't consider promise runtimes #25

Closed johanatan closed 1 month ago

johanatan commented 1 month ago

hi,

in my project, i had the following function which I used to time the true runtime of promises (nested arbitrarily deep). can you see any way to do something similar with tel/trace!? i would love to replace it with telemere in my codes.

thanks again for such a great piece of software!

(defn ptime- [label p]
  (p/let [start (p/resolved (js/performance.now))
          res p]
    (js/console.debug (format "%s: Elapsed time: %.6f msecs" label (- (js/performance.now) start)))
    res))
ptaoussanis commented 1 month ago

@johanatan Hi Johanatan,

So Telemere's trace! runtime measurement is intentionally very simple - it just captures the start and end time of the wrapped form.

This means that anything in the form that's lazy or async intentionally won't contribute to the runtime (since it's not actually affecting the runtime of that thread at that time).

Tufte has some relevant info about this which I'll copy over to the relevant Telemere docstrings.

I'm not too sure I understand from your example exactly what you'd like to achieve with Telemere, sorry. Could you maybe show an example of the code you want to use, and what result you'd like to see?

In general you've got 2 options for measuring the runtime of async/lazy code:

  1. Make sure that you wrap where the cost is actually realized (e.g. where your promise is resolved), or
  2. Manually measure your runtime using whatever tools you prefer, then provide that info to Telemere as data

For (2), I'd note that Tufte is one option that works well with Telemere - and that'll share the same filtering + handler API after v3 expected later this/next month.

Is that any help?

johanatan commented 1 month ago

1 is not possible because a single p/let (from promesa) will represent n promise bindings, each of which could be their own p/let internally) hence "arbitrarily deep".

A typical use of ptime is just like the use of tel/trace!:

(ptime (p/let [...] res))

Please let me know if that is still not clear. I think it would also help to read the definition of ptime and understand what it is doing. I understand that my request is not superficial.

johanatan commented 1 month ago

Regarding your second suggestion, I think I could just modify ptime to pass the computed run time to telemere instead of printing it using js/console. That's an obvious solution here but I figured you may have something deeper if you've thought about this case.

If I end up doing this, would you be interested in accepting a PR with it? I believe it can be made promise library agnostic by accepting JS promises (which promesa's p/let does in fact).

ptaoussanis commented 1 month ago

What is the p namespace in p/let, p/resolved?

johanatan commented 1 month ago

https://github.com/funcool/promesa

But if I did this as a PR, that would be an implementation detail you wouldn't have to worry about. It would just be one extra dependency in your project lein or deps.edn.

The advantage to (myself) & users would be that they could do the following:

(tel/trace! {...} p)

Where p is any standard JavaScript promise and the runtime would be captured regardless how arbitrarily deep p is nested with other promises.

We could of course make it a separate function like:

(tel/ptrace! ...) also if you prefer.

ptaoussanis commented 1 month ago

https://github.com/funcool/promesa

Thanks 👍 Please keep in mind that not everyone is familiar with the same libraries. I get so many folks mentioning or including code from libraries that I've never heard of or used - it's really helpful to have non-core libraries explicitly linked to avoid confusion or wasted effort.

As in this case, I'm not familiar with the lib or what it's doing - will try take a look later this evening or at the weekend and come back to you.

But if I did this as a PR, that would be an implementation detail you wouldn't have to worry about. It would just be one extra dependency in your project lein or deps.edn.

I'm definitely not keen on any dependencies. Before having that talk, I'd prefer to start with a clear description of the problem and objectives. Will come back to you when I've had a chance to investigate.

johanatan commented 1 month ago

Ok, this could be done with the raw JavaScript promise as well in that case.

Please let me know if you would accept a PR using only the raw JavaScript promise to implement this.

Thanks!

ptaoussanis commented 1 month ago

Please note that I did mention the word "promesa" several times (including in the original) & I figured one would do a search if not familiar.

Your original post didn't make any mention of the library you were using, that's why I asked what was meant by the p/ namespace.

ptaoussanis commented 1 month ago

Please let me know if you would accept a PR using only the raw JavaScript promise to implement this.

I'd be happy to look at a PR since that'd also help give me a better idea of what you have in mind - but please note that I'm not clear yet on what the problem or objectives are, so I can't promise anything about whether it'd make sense to add to Telemere yet.

johanatan commented 1 month ago

This is about JavaScript promises, not promesa. And an LLM could explain this all to you. If you pasted the title of this issue and the very first message to you, character for character, into an LLM, it would elaborate for you.

But it's clear that you are not that up to speed on promises. Which is fine but it is a very core idea at the heart of modern JavaScript and it is not at all unreasonable for me to assume that someone who has a library that targets a language hosted on JavaScript would be familiar.

But once again, LLMs are your friend here.

ptaoussanis commented 1 month ago

And an LLM could explain this all to you. If you pasted the title of this issue and the very first message to you, character for character, into an LLM, it will elaborate for you.

I'm sorry, but I'm not going to enter your questions into an LLM to have the LLM try make sense of them and add basic context like what library we're talking about. You're literally using code with undefined aliases, and arguing that it's my responsibility to figure out what you mean instead of just asking you what the aliases are for?

But it's clear that you are not that up to speed on promises.

And it's clear that we have a different opinion about polite discourse. I'm not supportive of this kind of entitled attitude toward open source maintainers. You're asking me for help here, please consider showing me at least the same level of respect that I'm showing you in taking the time to respond.

johanatan commented 1 month ago

I'm not asking for help here. I'm offering an improvement to your project. Even just the creation of this issue itself was a free contribution that I made and that you had no right to but I went beyond that to even offer the PR itself. You are the one making all this fuss about my communication not being in the form you like. That seems like the height of entitlement from my view.

And also the attitude that you think that everyone who posts anything here at all is "asking for help" is a sign of arrogance-- that you know better than they possibly could. And that is also perhaps why this is resulting in a clash here.

ptaoussanis commented 1 month ago

You are the one making all this fuss about my communication not being in the form you like. That seems like the height of entitlement from my view.

The only "fuss" I made was to very politely suggest/request that in future when you use non-core libraries in code snippets, you take a small moment to please mention what library you're using so that the person responding to your issue has the proper context.

It wasn't initially clear to me what p/let or p/resolved referred to since you hadn't initially provided that information.

That you view this request for basic context as "the height of entitlement", and then tell me to use an LLM to try infer your aliases is baffling. "LLMs are your friend", really?

[...] And also the attitude that you think that everyone who posts anything here at all is "asking for help" is a sign of arrogance [...] But it's clear that you are not that up to speed on promises. Which is fine but it is a very core idea at the heart of modern JavaScript and it is not at all unreasonable for me to assume that someone who has a library that targets a language hosted on JavaScript would be familiar.

I'm sorry, but the trajectory this has taken strikes me as unnecessarily hostile and mean-spirited.

I guess we have a difference of opinion on the situation. I believe that you're being rude, and you clearly believe that I'm being rude.

Neither of us seems to be benefitting from the exchange, so let's part ways amicably. I really do appreciate the time you've taken to post this, and I do apologise if I have been unreasonable.

In any event, I'll be bowing out of future interactions from here.

All the best, sincerely.

ptaoussanis commented 1 month ago

Closing for now, but if anyone else reads this and is interested in the same topic - please feel free to open a new issue and I'll investigate 👍

johanatan commented 1 month ago

For anyone following along at home, the following is what I came up with:

(require [promesa.core :as p])

(defn ptrace!
  ([label p]
   (ptrace! label p {}))
  ([label p data]
   (p/let [start (js/performance.now)
           res p
           run-msecs (- (js/performance.now) start)]
     (tel/log! {:level :info :data (merge data {:run-msecs run-msecs}) :print-console? true}
               (format "%s: Elapsed time: %.2f msecs" label run-msecs))
     res)))

Please note that this situation would be far better if tel/trace! itself detected when the return value of the run-form is a promise, and resolved it automatically (recording the run time as per normal for trace! and spy!). The downside to my wrapper is that now there are two ways of tracing / logging in the codebase and that just isn't as elegant and doesn't read as well. Also, this method has no way to capture the run-form itself (for that you'd need a macro, which tel/trace! already is).

Further, in the modern JavaScript landscape, promises are quite common and they are always handled transparently in this fashion by properly designed codes. There is a prevalance of async/await usage and functions marked as async make up large swaths of modern codebases (which translates to returning promises [hidden by the async/await syntax in JavaScript, which is nicely simulated via the promesa library, and others, in ClojureScript]). For these reasons, I do not think a library such as Telemere that targets the JavaScript ecosystem should consider "async" an afterthought or non-normative.

johanatan commented 1 month ago

Note that we have a very similar structure here in the telemere source codes: https://github.com/taoensso/telemere/blob/master/projects/main/src/taoensso/telemere/impl.cljc#L726-L729

and I believe this would be the place to make such a change, line 728 to be precise. It is nestled very deep with very complicated, and repetitious, macro codes that I did not really want to get my hands dirty with, so it is left as an exercise for the reader (or author of this library).

ptaoussanis commented 1 month ago

For these reasons, I do not think a library such as Telemere that targets the JavaScript ecosystem should consider "async" an afterthought or non-normative.

There is a lot of nonsense going on in this thread. Firstly, "async" is not identical to JS promises. Secondly, I never once suggested that there's anything wrong with considering how Telemere might best support JS promises. I specifically said that I would investigate that evening or at the weekend.

Step 1 in any issue is agreeing on a clear description of the problem and objectives.

But you started the discussion by referencing code from an initially unidentified library that I'm unfamiliar with (Promesa). I said I'd take a look to familiarize myself before commenting, and requested that in future you please identify libraries used in code snippets.

Then the discussion got derailed by the latter request, and devolved into what we're both interpreting as rudeness by the other.

[...] promises are quite common and they are always handled transparently in this fashion by properly designed codes [...] it is nestled very deep with very complicated, and repetitious, macro codes

Look, it seems to be that we're now just operating in bad faith - so I'm unfortunately bowing completely out of further interactions with you.

This isn't at all personal, I just don't have an overabundance of free emotional energy atm - and this has been and continues to be unnecessarily draining. To be honest I'd rather focus my limited energies elsewhere. I accept that I may be in the wrong here, apologies if that's the case.

If anyone else is interested in this topic, they're of course very welcome to create a new issue and I'll happily investigate options. Not doing it right now purely because I have 20 other things that I'm working on, and prioritise topics based on demand.