Fix Graphite Tag integration by sarthaksin1857 · Pull Request #131 · apache/otava

sarthaksin1857 · 2026-02-22T19:02:30Z

What

Change example image to alpine and install bash/curl (datagen was not working when curl was uncommented)
Make all fields optional with default values in GraphiteEvent

Proof it works

With the graphite example in the repo, we now see

time                       run    branch       version    commit      throughput    response_time    cpu_usage
-------------------------  -----  -----------  ---------  --------  ------------  ---------------  -----------
2026-02-24 04:29:10 +0000         new-feature  0.0.1      p7q8r9           61160               87          0.2
2026-02-24 04:30:10 +0000         new-feature  0.0.1      m4n5o6           60160               85          0.3
2026-02-24 04:31:10 +0000         new-feature  0.0.1      j1k2l3           60960               89          0.1
                                                                    ············                   ···········  
                                                                           -5.6%                       +300.0%  
                                                                    ············                   ···········  
2026-02-24 04:32:10 +0000         new-feature  0.0.1      g7h8i9           57123               88          0.8
2026-02-24 04:33:10 +0000         new-feature  0.0.1      d4e5f6           57980               87          0.9
2026-02-24 04:34:10 +0000         new-feature  0.0.1      a1b2c3           56950               85          0.7

Which makes sense as we do not set run but we do set everything else

Fix data generation script to send required fields nit remove non graphite changes nit Add datagen changes Fails still but can deploy local code Set defaults for all variables and manually specify pub_time Add table on supported metadata field nit Only confirm pub_time is set properly

sarthaksin1857 · 2026-02-24T04:41:08Z

@Gerrrr Can you take a look here

Gerrrr

Thanks for opening this PR, @sarthaksin1857 ! I found a few things we need to fix before it is ready to merge. Also, given the wealth of subtle issues, this class may deserve a few unit tests.

Gerrrr · 2026-02-27T03:04:30Z

 filter data by commit or version using `--since-commit` or `--since-version` selectors.

+#### Supported Metadata Schema
+The following keys are supported within the `data` dictionary:


It may not be clear what is the data dictionary in this case. In the paragraph above, we talk about tags, so I suggest replacing this statement with The following tags are supported:

Gerrrr · 2026-02-27T03:13:25Z

+        if isinstance(self.pub_time, str):
+            self.pub_time = parse_datetime(self.pub_time)
+        elif isinstance(self.pub_time, (int, float)):
+            self.pub_time = datetime.fromtimestamp(self.pub_time)


This change uses a different parsing logic than parse_datetime. The latter does dateparser.parse(date, settings={"RETURN_AS_TIMEZONE_AWARE": True}).

Why do need these extra manipulations with pub_time? I don't follow how making fields optional required it.

This was more just me looking at this code an how to future proof it.

The function says that pub_time is already a date time and if its already a datetime, why try to parse it again?

Similarly if someone passes in a unix int like 1776522767 the string function wont work either

So before we passed in an integer string as "datetime" and then parsed it back into a string, and then into the proper object.

I will just move the logic into parse_datetime and make this code cleaner

Gerrrr · 2026-02-27T03:14:54Z

-            self.version = None
-        else:
-            self.version = version
-        if len(branch) == 0 or branch == "null":


This logic was lost in this PR. If a string text is empty or equal to "null", we still want to treat it as empty, unless there is a reason not to.

Since I switched all actual "optional" fields to be optional, I will clean all of them (except pub_time)

Gerrrr · 2026-02-27T03:17:23Z

-        end_time: int,
-        version: Optional[str],
-        branch: Optional[str],
-        commit: Optional[str],


idea (maybe outside of scope of this PR): today, if we have other event tags like "environment": "prod", we'll crash with unexpected keyword argument. Maybe we can change this class such that it'd work arbitrary tags.

Yea I wonder if we should even define fields here as "supported tags" and rather just store them in a map and print them all at the end as columns

I like this idea! Again, it is not required in this PR, but is worth doing in a follow-up.

documentation fixup Address comments Add coverage for parsing Clean all string Indentation nit formatter Auto formatter

sarthaksin1857 · 2026-04-18T16:05:57Z

@Gerrrr sorry for the wait this is ready for another pass.

I made it so

All optional fields are cleaned
the parse_datetime utility handles string, int, long, and datetime inputs
Updated the documentation
Added a few tests

I tried to extend the integration tests but it didn't seem to play well with tags. Whenever I tried sending an event, it would time out (I can file an issue for adding integration coverage and tackle that next?)

Gerrrr

Apologies for the long review cycle. This PR is almost ready - just a few small things to fix. Can you also paste the WIP integration test that timed out for you? Maybe I'll be able to help out with it.

Gerrrr · 2026-05-21T03:40:09Z

+    if isinstance(date, (int, float)):
+        return datetime.fromtimestamp(date, tz=timezone.utc)
+
+    if isinstance(date, str):


This method supports strings, ints, floats, datetime now. However, all its callers are using str(input), so there are no callers at all that benefit from this change.

While I am not asking to refactor callers of this method across the entire codebase, do you think it would make sense to use it in Graphite importer? If not... then what's the point of this change?

Gerrrr · 2026-05-21T03:52:30Z

+        return None
+
+    if not isinstance(value, str):
+        return value  # or raise TypeError if you want strictness


This return violates the type signature. I'd throw a type error.

Gerrrr · 2026-05-21T03:53:28Z

-        end_time: int,
-        version: Optional[str],
-        branch: Optional[str],
-        commit: Optional[str],


I like this idea! Again, it is not required in this PR, but is worth doing in a follow-up.

Gerrrr · 2026-05-21T03:57:31Z

+        if self.pub_time is None:
+            raise ValueError("pub_time is required and cannot be None")
+        # Ensure pub_time is always a datetime
+        self.pub_time = parse_datetime(str(self.pub_time))


I think we should do parse_datetime on start_time and date_time. If they are None, they will remain None. If they are strings/ints, they will get converted to datetime. Without this change, we may get arbitrary types in those fields. Example:

def test_graphite_event_parses_start_and_end_time(): """start_time and end_time are documented as supported tags and annotated Optional[datetime]; if Graphite delivers them as Unix timestamps (int) or ISO strings in the event data, they must be normalized to tz-aware datetimes, the same way pub_time is.""" event = GraphiteEvent( pub_time=1700000000, start_time=1700000000, end_time="2024-01-01 10:00:00", ) assert isinstance(event.start_time, datetime) assert event.start_time.tzinfo is not None assert isinstance(event.end_time, datetime) assert event.end_time.tzinfo is not None

sarthaksin1857 force-pushed the fix-graphite-datagen branch from f89d7e2 to 8f70419 Compare February 22, 2026 19:11

sarthaksin1857 force-pushed the fix-graphite-datagen branch from ec4a46e to adc9f02 Compare February 24, 2026 04:38

sarthaksin1857 marked this pull request as ready for review February 24, 2026 04:40

Gerrrr requested changes Feb 27, 2026

View reviewed changes

sarthaksin1857 force-pushed the fix-graphite-datagen branch from a76be64 to 2b019bf Compare April 18, 2026 15:45

make defaults none

19e758c

documentation fixup Address comments Add coverage for parsing Clean all string Indentation nit formatter Auto formatter

sarthaksin1857 force-pushed the fix-graphite-datagen branch from c0ea484 to 19e758c Compare April 18, 2026 15:56

indentation stuff

bfb7c01

Fix my own tests

f3b786a

Gerrrr requested changes May 21, 2026

View reviewed changes

Gerrrr self-assigned this May 30, 2026

Uh oh!

Conversation

sarthaksin1857 commented Feb 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Proof it works

Uh oh!

sarthaksin1857 commented Feb 24, 2026

Uh oh!

Gerrrr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sarthaksin1857 commented Apr 18, 2026

Uh oh!

Gerrrr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sarthaksin1857 commented Feb 22, 2026 •

edited

Loading