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

Including list of common units for formatting purposes in Backend. #19138

Merged
merged 12 commits into from May 15, 2024

Conversation

luk-kaminski
Copy link
Contributor

@luk-kaminski luk-kaminski commented Apr 23, 2024

Description

Hardcoded list of supported units.
Fields can have unit associated with them.
Some basic GL fields get default/hardcoded unit association.
/nocl

Fixes #19157
Fixes #19299

Motivation and Context

  1. Do not hardcore units in FE.
  2. Encode conversion of units inside them.
  3. Group units by types.
  4. Use units for some fields without a need to configure anything in the widget.

How Has This Been Tested?

Manually and with new unit tests.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Refactoring (non-breaking change)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.

@bernd
Copy link
Member

bernd commented Apr 23, 2024

What's the use case for storing the units in the database? There aren't that many units for data values. Why not just hardcode them in the UI code?

@luk-kaminski
Copy link
Contributor Author

What's the use case for storing the units in the database? There aren't that many units for data values. Why not just hardcode them in the UI code?

We would like to avoid the necessity to change the code if a new unit is needed. We hope it can be just added to DB and the rest will work transparently.

We also consider storing field-unit relation, especially for GL own fields, but probably for all other fields as well.

@luk-kaminski luk-kaminski marked this pull request as ready for review April 24, 2024 11:51
@bernd
Copy link
Member

bernd commented Apr 24, 2024

We would like to avoid the necessity to change the code if a new unit is needed. We hope it can be just added to DB and the rest will work transparently.

Hmm, can't we check which units would be needed and ship a pre-defined list? There aren't that many different common units. That would avoid the database collection, API endpoint, and the code for all of that.

We also consider storing field-unit relation, especially for GL own fields, but probably for all other fields as well.

Okay, got it. How about pre-define the units (enum?) and then use it for the field-unit relation. For the UI we could use our existing infrastructure to generate TypeScript files for the pre-defined units so can maintain them in a single place.

What do you think?

@luk-kaminski
Copy link
Contributor Author

luk-kaminski commented Apr 24, 2024

Hmm, can't we check which units would be needed and ship a pre-defined list?

We can, but then we end up with a close list of units. With this approach if someone needs a new set of units, it is easy to expand and upgrade. We can add migrations to add new unit sets, throw some new units manually into DB if rich client pays for this, or even create a UI for importing/creating additional unit at some point in the future, if needed.

There aren't that many different common units. That would avoid the database collection, API endpoint, and the code for all of that.

Is it really problematic that we have a new collection? It won't be big, so I assume it is not storage that you are worried about, but the existence of the collection itself?
When it comes to the code, probably a similar amount of code would be needed in the FE if we moved it there (now Resource class is tiny, so is Service if you exclude derived units generation for basic prefixes (giga, mega, nano etc.), which would need to be repeated, or hardcoded in FE anyway with your approach ).

Okay, got it. How about pre-define the units (enum?) and then use it for the field-unit relation. For the UI we could use our existing infrastructure to generate TypeScript files for the pre-defined units so can maintain them in a single place.

You mean that somewhere in the DB we would refer to FE-only enum values?

@luk-kaminski
Copy link
Contributor Author

luk-kaminski commented Apr 24, 2024

@bernd - FYI, we are supposed to provide around 13 units initially.
Imho it is quite likely new units may be requested by customers (length, imperial length, temperature, transfer(in bits?), weight), so if they liked the feature and if we have indeed a variety of use-case scenarios/users, the list could easily cover 50-100+ units.
My attempt was to be ready for extension without significant difficulties.

@bernd
Copy link
Member

bernd commented Apr 24, 2024

@luk-kaminski
Copy link
Contributor Author

I doubt that we need more than this: https://github.com/grafana/grafana/blob/2d9d0e61b132b3eecc7a9c61a72235dbd19d9711/packages%2Fgrafana-data%2Fsrc%2FvalueFormats%2Fcategories.ts#L37-L439

😄

Heh, it seems they don't even support auto-scaling yet, at least not in this file. ;)

@luk-kaminski
Copy link
Contributor Author

@bernd - in order to proceed with the topic, I could also introduce a second implementation of the unit service, in memory one, and work on that in the near future. It would be faster than our current discussion and we will be able to switch to the db service if I managed to persuade you that we need it at some point.

What do you think? :)

@bernd
Copy link
Member

bernd commented Apr 25, 2024

Sorry for the sluggish responses. I was at an off-site meeting.

Is it really problematic that we have a new collection? It won't be big, so I assume it is not storage that you are worried about, but the existence of the collection itself?
When it comes to the code, probably a similar amount of code would be needed in the FE if we moved it there (now Resource class is tiny, so is Service if you exclude derived units generation for basic prefixes (giga, mega, nano etc.), which would need to be repeated, or hardcoded in FE anyway with your approach ).

The additional database collection is not the issue. To me, making a finite and well-known collection of units configurable via the database and having an API for them introduces unneeded complexity.

I think there is a limited number of units that make sense in a Graylog context. It's also easy to add more in a new release when customers request them. (also easier to backport when we only have to maintain a static list) Until now we don't have any units, so it's probably okay for customers to wait for the next release to get more exotic ones. 😄

You mean that somewhere in the DB we would refer to FE-only enum values?

What I had in mind was to define the units in a resource file. For the frontend, we could generate TypeScript code from that resource file at build time. That way, we don't need an API endpoint and HTTP requests to get the available units. Similar to how we generate TypeScript code for HTTP API models.

@bernd - in order to proceed with the topic, I could also introduce a second implementation of the unit service, in memory one, and work on that in the near future. It would be faster than our current discussion and we will be able to switch to the db service if I managed to persuade you that we need it at some point.

If you and your team think we need the ability to make units configurable via the database and it's worth the added complexity, you should continue with your current approach.

@bernd
Copy link
Member

bernd commented Apr 29, 2024

@luk-kaminski If you continue with the database approach, please remember to implement content-pack support for units and add dependency resolution for the view entities. Otherwise, the referenced units in views might be broken.

@luk-kaminski
Copy link
Contributor Author

@luk-kaminski If you continue with the database approach, please remember to implement content-pack support for units and add dependency resolution for the view entities. Otherwise, the referenced units in views might be broken.

We are not sure about the decision yet. Will hopefully have a meeting tomorrow.

@dennisoelkers
Copy link
Member

@luk-kaminski, @bernd: I would say we should move these definitions to the code. Previously, I was torn between the complexity of the implementation and the fact that it is already implemented, but the fact that we would still need to add content pack support pushes me over the edge towards a simpler solution.

@luk-kaminski luk-kaminski marked this pull request as draft May 6, 2024 09:44
@luk-kaminski luk-kaminski changed the title Units storage in MongoDB. Units support - BE May 7, 2024
@luk-kaminski luk-kaminski marked this pull request as ready for review May 8, 2024 14:07
Copy link
Member

@dennisoelkers dennisoelkers left a comment

Choose a reason for hiding this comment

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

First round of feedback, as this will probably require some changes that affect the rest of the review.

Removed REST endpoints and further remainings of Mongo code. Added JSON file as a source of suported units.

Copying units json for FE in mvn compile phase

Folder for common resources. Data moved from there to BE resources, by Maven

With units hardcoded in file, some functionality does not have sense anymore and was removed

Units are referred by identifiers

List of fields with default units prepared
Copy link
Member

@dennisoelkers dennisoelkers left a comment

Choose a reason for hiding this comment

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

Looks much better. Added some more inline comments for improvements.

@dennisoelkers dennisoelkers changed the title Units support - BE Including list of common units for formatting purposes in Backend. May 14, 2024
Copy link
Contributor

@janheise janheise left a comment

Choose a reason for hiding this comment

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

LGTM!

@janheise janheise merged commit aa5dba0 into master May 15, 2024
7 checks passed
@janheise janheise deleted the feature/formatting_units branch May 15, 2024 09:20
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.

Prepare a list of predefined fields, that should have their units hardcoded. Unit and unit type storage
4 participants