I guess this should also state the license under which this code is included here? Or a LICENSE file in the same directory?

Added the MIT license to jquery.flot.heatmap.js

Now I'm a bit confused because VictoriaMetrics is Apache 2 licensed and not MIT.

I first thought that maybe the part of the codebase where this file is copied from is MIT licensed. But I couldn't even find a file that is the same (or at least similar) to this one.

Where did you copy this file from?

Or alternatively, if you created this file just for this PR, then we don't really need to put it in a vendor directory (as it isn't vendored), and we can use the usual Apache 2 license and don't need a copyright header.

This file was created for this PR. I saw that all plugins for flot were in this folder, so I placed it there. I can create a new folder, for example flot/plugins, and move it there, or you can suggest a better place for it.

I see… On the other hand, the other files are actually directly copied from another repository.

Not sure what to do. I'm not very familiar with the UI code. @juliusv could you advise how to handle the case where a flot plugin has been created originally and not just copied from another source? Should we still have it here or in another directory?

@Nexucis might also be able to help here.

I don't mind this particular part too much, IMO we can keep it in this location for now.

yeah I agree we can keep it here. For the moment I'm just sad to see another file going there since my best wish so far is to drop this vendor folder. But that's definitely another subject totally out of scope of this PR

Here and elsewhere: We generally use a style where we always use curly braces and separate lines for blocks, even if they are just one line like here. That way it's easier to see where control flow happens and easier / less error-prone to extend when necessary.

I think we don't need an array here - that was only necessary in the VictoriaMetrics case, where they have multiple different histogram labels. I'd just go with const bucketLabel = 'le';.

Would make the code easier to read too, for example:

result.every((r) => histogramLabels.some((l) => l in r.metric));

would become

result.every((r) => bucketLabel in r.metric));

or since you do it in other places already, feel free to hardcode the le instead of having a constant for it:

result.every((r) => r.metric.le));

IMO that's even clearer to read :)

I think this can just be:

Since you have index = 0 and index += 1, how about using a classic for loop while at the same time eliminating the need for a prevBucket variable, like this?

  for (let i = 0; i < sortedBuckets.length; i++) {
    const values = [];
    const { data, labels, color } = sortedBuckets[i];

    for (const [timestamp, value] of data) {
      const prevVal = sortedBuckets[i - 1]?.data.find((v) => v[0] === timestamp)?.[1] || 0;
      const newVal = Number(value) - +prevVal;
      values.push([Number(timestamp), newVal]);
    }

    result.push({
      data: values,
      labels,
      color,
      index: i,
    });
  }

I'd prefer calling this heatmap or histogramHeatmap for clarity in all the dependent code that I read. Just histogram makes me think that it just means that the underlying data is a histogram, or if I think about it as a display option, that it means a regular histogram bar chart.

This clarifies to me that stacked is not remembered across switching into heatmap mode and back - I think that's ok, but then it would (at least eventually) make more sense to replace the stacked boolean option with a displayMode: 'lines' | 'stacked' | 'heatmap' option. Then we don't have multiple option fields where some values might be invalid state combinations.

Instead of enabling/disabling this button, I'd prefer only showing it at all when the data is a histogram. That way users who never deal with histograms don't even have to think about it, while users who do deal with histograms will notice it even more when it only appears for histogram data.

We will always want to encode all available graph options in the URL so that we can send around heatmap graphs as well, so we'll want to encode the new display mode setting as well.

I'd go for this approach:

• On reading, be backwards-compatible with the legacy stacked parameter and just translate it to the new display mode setting as you are doing it here already.
• On writing the options back to the URL, only store the display mode parameter (maybe display_mode=lines|stacked|heatmap?) and omit stacked.

If a URL happens to somehow contain both the legacy stacked parameter and display_mode, then display_mode can take precedence.

That way a new Prometheus version can still open the old URLs correctly, but will produce the new format going forward.

What do you think, what's the better practice for test cases: using string literals or enums for parameters like 'display_mode'?

.toEqual(...g0.display_mode=${GraphDisplayMode.Stacked}&...);
or
.toEqual(...g0.display_mode=stacked&...);"

Yeah, I get the question for tests, but I don't think it makes much of a difference in this kind of case, so I'm happy with either.