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

hotsos/core: add name aliasing support #929

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

xmkg
Copy link
Contributor

@xmkg xmkg commented Jul 5, 2024

at the moment, scenarios are using the long import paths in order to reference to a Python property. this feature allows assigning an alias to a Python class or variable in order to make using plugin property interfaces easier.

Added aliases to the plugins.
Updated existing scenarios to use aliases.

Fixes #912

@xmkg xmkg requested a review from dosaboy July 5, 2024 15:58
@xmkg xmkg force-pushed the feature/alias branch 2 times, most recently from af813d6 to 403c38c Compare July 7, 2024 10:58
@xmkg xmkg force-pushed the feature/alias branch 3 times, most recently from c4a9496 to ddc4d2a Compare July 18, 2024 08:48
Copy link
Member

@pponnuvel pponnuvel left a comment

Choose a reason for hiding this comment

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

The aliasing naming seems a bit odd to me. For example, juju.base, kernel.calltrace, ceph.daemon.all-osds and so on give the impression it's a package/class hierarchy when they are just a given name.

Given we could mix with the original way of writing/accessing the properties, the use of dots seems a little misleading.

Perhaps we can drop the dot (A.B) in aliases and use a single word name such as jujuBase/jujubase/juju_base or something like that?

@@ -63,6 +65,7 @@ def path(self):
return os.path.join(HotSOSConfig.data_root, 'proc/vmstat')

@property
@alias("kernel.vmstat.compaction_failures_pct")
Copy link
Member

Choose a reason for hiding this comment

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

I think this isn't needed since the class is aliased. Can be directly used as kernel.vmstat.compaction_failures_percent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice catch, I'll remove it

@xmkg
Copy link
Contributor Author

xmkg commented Jul 26, 2024

The aliasing naming seems a bit odd to me. For example, juju.base, kernel.calltrace, 'ceph.daemon.all-osds` and so on give the impression it's a package/class hierarchy when they are just a given name.

Given we could mix with the original way of writing/accessing the properties, the use of dots seems a little misleading.

The dot separation represents a hierarchy, where the first part is the plugin name, then the optional subsections, and finally the property name. I made it this way so if we write a YAML schema(*another important QOL improvement that will make it easier to write scenarios) for the scenarios for IDE/editor assistance in the future, it would be convenient for tab completion.

As for the possible confusion, we can easily distinguish an alias from the full path as the package names always have to start with hotsos. (*unless we start supporting external plugins), so if it starts with "hotsos", it's a full property path, otherwise, it's an alias.

@pponnuvel
Copy link
Member

The dot separation represents a hierarchy, where the first part is the plugin name, then the optional subsections, and finally the property name.

Now you say that, it makes sense. But it's not obvious otherwise. Also nothing prevents anyone from giving arbitrary names. So might be useful to have a section ("Alias naming") in documentation on the naming convention.

@xmkg xmkg force-pushed the feature/alias branch 2 times, most recently from 43cb969 to f5221a8 Compare July 30, 2024 10:47
@xmkg
Copy link
Contributor Author

xmkg commented Jul 30, 2024

So might be useful to have a section ("Alias naming") in documentation on the naming convention.

Added a check for preventing an alias from starting with "hotsos.", added the "Alias naming" section to the docs, and also added some unit tests to verify all that.

at the moment, scenarios are using the long import paths in order to
reference to a Python property. this feature allows assigning an alias
to a Python class or variable in order to make using plugin property
interfaces easier.

Added aliases to the plugins.
Updated existing scenarios to use aliases.
Added unit tests for the aliasing code.
Updated the docs.

Fixes canonical#912

Signed-off-by: Mustafa Kemal Gilor <[email protected]>
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.

Add property aliasing to Python properties
2 participants