Skip to content

Conversation

@jcolladokuri
Copy link

@jcolladokuri jcolladokuri commented Nov 7, 2025

This PR adds support for PDC on the GitHub datasource:

Screenshot 2025-11-07 at 10 12 06 AM

I tested using: https://wiki.grafana-ops.net/w/index.php/Engineering/Grafana/Data_Sources/API_servers/Testing_datasources_with_PDC_Locally.

Confirmed that after enabling PDC, and the secure socks proxy toggle, I could see logs coming from the Agent. Similarly when I turned the agent off, I could see queries begin to fail.

Fixes #120

@jcolladokuri jcolladokuri requested a review from a team as a code owner November 7, 2025 16:14
@CLAassistant
Copy link

CLAassistant commented Nov 7, 2025

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@zoltanbedi zoltanbedi left a comment

Choose a reason for hiding this comment

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

Couple of things but I tried out and it works! Great job. Also please sign the cla again.

)}
</ConfigSection>

{config.featureToggles['secureSocksDSProxyEnabled' as keyof FeatureToggles] && gte(config.buildInfo.version, '10.0.0') && (
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{config.featureToggles['secureSocksDSProxyEnabled' as keyof FeatureToggles] && gte(config.buildInfo.version, '10.0.0') && (
{config.secureSocksDSProxyEnabled && (

Copy link
Author

Choose a reason for hiding this comment

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

So I tried changing this to only checking config.secureSocksDSProxyEnabled but this does not seem to hide the UI portion for secure socks:

Here is the feature toggle for secureSocksDSProxyEnabled disabled:

Screenshot 2025-11-10 at 11 47 51 AM

But in the UI I can still see an option for Secure Socks:

Screenshot 2025-11-10 at 11 49 15 AM

datasourceSettings.CachingEnabled = true

instance, err := NewGitHubInstance(context.Background(), datasourceSettings)
instance, err := NewGitHubInstance(context.Background(), datasourceSettings, opts)
Copy link
Member

Choose a reason for hiding this comment

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

Do you know perhaps why we don't pass down the context from the NewDataSourceInstance?

Copy link
Author

Choose a reason for hiding this comment

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

I am honestly not quite sure why we create a new separate "immortal" context.

I changed this to use the ctx from NewDatasourceInstance and it all seemed to work as expected?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

4 participants